Fix attach goroutine/fd leak when no I/O is ready #45052

cpuguy83 · 2023-02-21T22:38:00Z

When only stdin is attached the goroutine can only ever exit if:

The container pipe is closed while trying to write to it
The client closes the stdin read pipe

This is because io.Copy does a read on the read side then a write to the write side.
If reading from the client's stdin pipe blocks, the goroutine will never get notified that the container pipe is closed.

Another similar case is when the client disconnects but io.Copy is blocking waiting for data from the container's stdout/stderr streams.
This is significantly trickier to resolve.
The 2nd commit fixes this for Linux only.

Fixes Docker daemon is leaking file descriptors (with reproduction) #37182

- Description for the changelog

fix a goroutine and file-descriptor leak on container attach

- A picture of a cute animal (not mandatory but encouraged)

api/server/router/container/container_routes.go

thaJeztah · 2023-02-27T18:44:54Z

/cc @corhere

corhere

I wonder if the client-disconnects solution is more clever than it needs to be. In the case of TCP, the lowest common denominator for connection state tracking, each peer can either gracefully close its write stream (FIN) or forcefully abort the whole connection (RST) but there is no affordance in the TCP state machine to signal gracefully that a peer is no longer interested in receiving any more data. And the only portable way to check whether a socket is still open is to try reading or writing at least one byte and see if it succeeds. What if we always used the client-closed-for-writing signal as our cue to stop copying stdout/stderr from the container? That is, have a goroutine which calls conn.Read() even when stdin isn't attached to the container, and closes stdout/stderr when the read returns io.EOF or similar. The same code path would work for UNIX domain sockets, TCP sockets, WebSockets and Windows named pipes; no platform-specific event handling required! And all the bases would be covered for TCP: FIN, RST and keep-alive timeout will all result in EOF on read.

api/server/router/container/notify_linux.go

cpuguy83 · 2023-02-27T23:56:38Z

@corhere You are right I think this could work.
I've added a 3rd commit with the changes.

container/stream/attach_test.go

cpuguy83 · 2023-03-03T23:25:27Z

So it seems like there is a problem with that 3rd commit, possibly because the read side is shutdown when there's nothing on stdin.

corhere · 2023-03-31T19:26:56Z

Sigh, the CLI half-closes the connection to signal EOF on stdin. So much for that idea. It's looking like the sorts of OS-specific close notifications you had implemented initially is really the only thing we could do to fix the leaks within the current API versions.

Using socket half-close to signal stdin closed rather than attach-done is arguably a design flaw in our container-attach protocol. With the WebSocket attach protocol also busted (ws provides framing but no multiplexing so stdout can't be distinguished from stderr) I see an opportunity to fix all these flaws... the third time around. I'm imagining new WebSocket attach protocol (paired with new endpoints?) for attach and exec which:

multiplexes output streams in a way which takes advantage of WebSocket framing (i.e. not stdcopy)
unconditionally multiplexes the output streams even when there is only a single output stream so that the daemon could signal the client out-of-band
also multiplexes stdin with out-of-band signaling for stdin-EOF and perhaps other things (SIGWINCH without making a separate request?)
But I digress...

cpuguy83 · 2023-07-05T23:01:07Z

Rebased this and removed the 3rd commit which didn't work as hoped.

corhere

I'm not super thrilled about the un-idiomatic use of callbacks in notifyClosed but I can understand why: the more idiomatic designs (synchronous blocking call returning an error, return a child context in the same vein as "os/signal".NotifyContext) would take a nontrivial amount of additional plumbing and refactoring. I have to concede that you have gone with the most pragmatic implementation.

LGTM!

api/server/router/container/notify_unsupported.go

api/server/router/container/notify_linux.go

corhere

cpuguy83 · 2024-03-21T19:00:35Z

Rebased and moved EINTR handlers to the unix_noeintr package.

neersighted

LGTM (with a couple nits), with a bit of being sad about not being able to solve this on Windows, but I don't begrudge not wanting to figure out a separate async I/O API.

daemon/attach.go

api/server/router/container/notify_unsupported.go

thaJeztah · 2024-03-26T08:04:33Z

Failure on rootless looks related; https://github.com/moby/moby/actions/runs/8428203898/job/23080313424?pr=45052#step:8:1254

=== FAIL: amd64.integration.container TestAttachDisconnectLeak (0.07s)
    attach_test.go:102: assertion failed: infoAttach.NGoroutines is <= info.NGoroutines

I kicked CI again to see if it's flaky

When only stdin is attached the goroutine can only ever exit if: 1. The container pipe is closed while trying to write to it 2. The client closes the stdin read pipe This is because `io.Copy` does a read on the read side then a write to the write side. If reading from the client's stdin pipe blocks, the goroutine will never get notified that the container pipe is closed. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2024-05-16T17:49:55Z

Updated the integration test to hopefully make it less likely to flake.

In cases where the client disconnects and there is nothing to read from a stdio stream after that disconnect, the copy goroutines and file descriptors are leaked because `io.Copy` is just blocked waiting for data from the container's I/O stream. This fix only applies to Linux. Windows will need a separate fix. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

This var for the incoming attach request. Just within this one function we also have `cfg`, and `ctr` already, so `c` just makes things more confusing. Not to mention `c` is usually referencing a container object in other parts of the code. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2024-05-24T22:32:10Z

Ok I think the test issue was that it was using the main integration daemon and the goroutine count was fluctuating at just the right times to break it.
I've switched it to use to use its own daemon to help reduce noise and that seems to have done the trick.

cpuguy83 · 2024-05-31T16:32:18Z

Ping, does this LGTY?

cpuguy83 · 2024-05-31T16:34:13Z

btw, I've opened a proposal on Go to extend the syscall.RawConn interface (or add an additional interface) to receive close notifications.
golang/go#67432

cpuguy83 · 2024-06-04T22:22:26Z

Ping

integration/container/attach_test.go

vvoland · 2024-06-05T11:35:22Z

api/server/router/container/notify_linux.go

+		err := unix_noeintr.EpollCtl(epFd, unix.EPOLL_CTL_ADD, int(fd), &unix.EpollEvent{
+			Events: unix.EPOLLHUP,
+			Fd:     int32(fd),
+		})
+		if err != nil {
+			log.G(ctx).WithError(err).Warn("notifyClosed: failed to register fd for close notifications")
+			return
+		}
+
+		events := make([]unix.EpollEvent, 1)
+		if _, err := unix_noeintr.EpollWait(epFd, events, -1); err != nil {


I don't think EPOLLHUP is emitted on a graceful close? Not 100% sure though, but in this case it would cause this goroutine to block indefinitely?

Should this also poll for EPOLLRDHUP?

If anything using EPOLL_CTL_ADD with EPOLLHUP is not needed because it always looks for EPOLLHUP events.
EPOLLRDHUP is sent when the peer closed the write side.
EPOLLHUP, as I recall, should be when the connection is gone.

I did a quick test with pure syscalls: https://gist.github.com/vvoland/cc9d31f0202cd233c2c2affd74294e6f

And it looks like shutdown + close on the client side doesn't trigger EPOLLHUP.

This might not actually be the same in our case since we're much higher up in the stack and there might be other things actually triggering the EPOLLHUP along the way.

But I must admit - I don't know exactly how EPOLLHUP works, so I'll just trust you if you say that it works as expected 👍🏻

I think EPOLLRDHUP would be like the tcp-half close situation which is why a simpler implementation that doesn't require epoll wouldn't work as was discussed above.

@vvoland this one LGTY?

My concern was that EPOLLHUP will not be actually emitted on a graceful close based on the C test code.
Just tested the PR though and it looks like it works fine, so 👍🏻

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/docker](https://togithub.com/docker/docker) | major | `26.1.4` -> `27.0.3` | --- ### Release Notes <details> <summary>docker/docker (docker/docker)</summary> ### [`v27.0.3`](https://togithub.com/moby/moby/releases/tag/v27.0.3) [Compare Source](https://togithub.com/docker/docker/compare/v27.0.2...v27.0.3) #### 27.0.3 For a full list of pull requests and changes in this release, refer to the relevant GitHub milestones: - [docker/cli, 27.0.3 milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.0.3) - [moby/moby, 27.0.3 milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.0.3) - Deprecated and removed features, see [Deprecated Features](https://togithub.com/docker/cli/blob/v27.0.3/docs/deprecated.md). - Changes to the Engine API, see [API version history](https://togithub.com/moby/moby/blob/v27.0.3/docs/api/version-history.md). ##### Bug fixes and enhancements - Fix a regression that incorrectly reported a port mapping from a host IPv6 address to an IPv4-only container as an error. [moby/moby#48090](https://togithub.com/moby/moby/pull/48090) - Fix a regression that caused duplicate subnet allocations when creating networks. [moby/moby#48089](https://togithub.com/moby/moby/pull/48089) - Fix a regression resulting in "fail to register layer: failed to Lchown" errors when trying to pull an image with rootless enabled on a system that supports native overlay with user-namespaces. [moby/moby#48086](https://togithub.com/moby/moby/pull/48086) ### [`v27.0.2`](https://togithub.com/moby/moby/releases/tag/v27.0.2) [Compare Source](https://togithub.com/docker/docker/compare/v27.0.1-rc.1...v27.0.2) #### 27.0.2 For a full list of pull requests and changes in this release, refer to the relevant GitHub milestones: - [docker/cli, 27.0.2 milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.0.2) - [moby/moby, 27.0.2 milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.0.2) - Deprecated and removed features, see [Deprecated Features](https://togithub.com/docker/cli/blob/v27.0.2/docs/deprecated.md). - Changes to the Engine API, see [API version history](https://togithub.com/moby/moby/blob/v27.0.2/docs/api/version-history.md). ##### Bug fixes and enhancements - Fix a regression that caused port numbers to be ignored when parsing a Docker registry URL. [docker/cli#5197](https://togithub.com/docker/cli/pull/5197), [docker/cli#5198](https://togithub.com/docker/cli/pull/5198) ##### Removed - api/types: deprecate `ContainerJSONBase.Node` field and `ContainerNode` type. These definitions were used by the standalone ("classic") Swarm API, but never implemented in the Docker Engine itself. [moby/moby#48055](https://togithub.com/moby/moby/pull/48055) ### [`v27.0.1`](https://togithub.com/moby/moby/releases/tag/v27.0.1) [Compare Source](https://togithub.com/docker/docker/compare/v26.1.4...v27.0.1-rc.1) #### 27.0.1 For a full list of pull requests and changes in this release, refer to the relevant GitHub milestones: - [docker/cli, 27.0.0 milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.0.0) - [moby/moby, 27.0.0 milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.0.0) - Deprecated and removed features, see [Deprecated Features](https://togithub.com/docker/cli/blob/v27.0.1/docs/deprecated.md). - Changes to the Engine API, see [API version history](https://togithub.com/moby/moby/blob/v27.0.1/docs/api/version-history.md). ##### New - containerd image store: Add `--platform` flag to `docker image push` and improve the default behavior when not all platforms of the multi-platform image are available locally. [docker/cli#4984](https://togithub.com/docker/cli/pull/4984), [moby/moby#47679](https://togithub.com/moby/moby/pull/47679) - Add support to `docker stack deploy` for `driver_opts` in a service's networks. [docker/cli#5125](https://togithub.com/docker/cli/pull/5125) - Consider additional `/usr/local/libexec` and `/usr/libexec` paths when looking up the userland proxy binaries by a name with a `docker-` prefix. [moby/moby#47804](https://togithub.com/moby/moby/pull/47804) ##### Bug fixes and enhancements - `*client.Client` instances are now always safe for concurrent use by multiple goroutines. Previously, this could lead to data races when the `WithAPIVersionNegotiation()` option is used. [moby/moby#47961](https://togithub.com/moby/moby/pull/47961) - Fix a bug causing the Docker CLI to leak Unix sockets in `$TMPDIR` in some cases. [docker/cli#5146](https://togithub.com/docker/cli/pull/5146) - Don't ignore a custom seccomp profile when used in conjunction with `--privileged`. [moby/moby#47500](https://togithub.com/moby/moby/pull/47500) - rootless: overlay2: support native overlay diff when using rootless-mode with Linux kernel version 5.11 and later. [moby/moby#47605](https://togithub.com/moby/moby/pull/47605) - Fix the `StartInterval` default value of healthcheck to reflect the documented value of 5s. [moby/moby#47799](https://togithub.com/moby/moby/pull/47799) - Fix `docker save` and `docker load` not ending on the daemon side when the operation was cancelled by the user, for example with <kbd>Ctrl+C</kbd>. [moby/moby#47629](https://togithub.com/moby/moby/pull/47629) - The `StartedAt` property of containers is now recorded before container startup, guaranteeing that the `StartedAt` is always before `FinishedAt`. [moby/moby#47003](https://togithub.com/moby/moby/pull/47003) - The internal DNS resolver used by Windows containers on Windows now forwards requests to external DNS servers by default. This enables `nslookup` to resolve external hostnames. This behaviour can be disabled via `daemon.json`, using `"features": { "windows-dns-proxy": false }`. The configuration option will be removed in a future release. [moby/moby#47826](https://togithub.com/moby/moby/pull/47826) - Print a warning when the CLI does not have permissions to read the configuration file. [docker/cli#5077](https://togithub.com/docker/cli/pull/5077) - Fix a goroutine and file-descriptor leak on container attach. [moby/moby#45052](https://togithub.com/moby/moby/pull/45052) - Clear the networking state of all stopped or dead containers during daemon start-up. [moby/moby#47984](https://togithub.com/moby/moby/pull/47984) - Write volume options JSON atomically to avoid "invalid JSON" errors after system crash. [moby/moby#48034](https://togithub.com/moby/moby/pull/48034) - Allow multiple macvlan networks with the same parent. [moby/moby#47318](https://togithub.com/moby/moby/pull/47318) - Allow BuildKit to be used on Windows daemons that advertise it. [docker/cli#5178](https://togithub.com/docker/cli/pull/5178) ##### Networking - Allow sysctls to be set per-interface during container creation and network connection. [moby/moby#47686](https://togithub.com/moby/moby/pull/47686) - In a future release, this will be the only way to set per-interface sysctl options. For example, on the command line in a `docker run` command,`--network mynet --sysctl net.ipv4.conf.eth0.log_martians=1` will be rejected. Instead, you must use `--network name=mynet,driver-opt=com.docker.network.endpoint.sysctls=net.ipv4.conf.IFNAME.log_martians=1`. ##### IPv6 - `ip6tables` is no longer experimental. You may remove the `experimental` configuration option and continue to use IPv6, if it is not required by any other features. - `ip6tables` is now enabled for Linux bridge networks by default. [moby/moby#47747](https://togithub.com/moby/moby/pull/47747) - This makes IPv4 and IPv6 behaviors consistent with each other, and reduces the risk that IPv6-enabled containers are inadvertently exposed to the network. - There is no impact if you are running Docker Engine with `ip6tables` enabled (new default). - If you are using an IPv6-enabled bridge network without `ip6tables`, this is likely a breaking change. Only published container ports (`-p` or `--publish`) are accessible from outside the Docker bridge network, and outgoing connections masquerade as the host. - To restore the behavior of earlier releases, no `ip6tables` at all, set `"ip6tables": false` in `daemon.json`, or use the CLI option `--ip6tables=false`. Alternatively, leave `ip6tables` enabled, publish ports, and enable direct routing. - With `ip6tables` enabled, if `ip6tables` is not functional on your host, Docker Engine will start but it will not be possible to create an IPv6-enabled network. ##### IPv6 network configuration improvements - A Unique Local Address (ULA) base prefix is automatically added to `default-address-pools` if this parameter wasn't manually configured, or if it contains no IPv6 prefixes. [moby/moby#47853](https://togithub.com/moby/moby/pull/47853) - Prior to this release, to create an IPv6-enabled network it was necessary to use the `--subnet` option to specify an IPv6 subnet, or add IPv6 ranges to `default-address-pools` in `daemon.json`. - Starting in this release, when a bridge network is created with `--ipv6` and no IPv6 subnet is defined by those options, an IPv6 Unique Local Address (ULA) base prefix is used. - The ULA prefix is derived from the Engine host ID such that it's unique across hosts and over time. - IPv6 address pools of any size can now be added to `default-address-pools`. [moby/moby#47768](https://togithub.com/moby/moby/pull/47768) - IPv6 can now be enabled by default on all custom bridge networks using `"default-network-opts": { "bridge": {"com.docker.network.enable_ipv6": "true"}}` in `daemon.json`, or `dockerd --default-network-opt=bridge=com.docker.network.enable_ipv6=true`on the comand line. [moby/moby#47867](https://togithub.com/moby/moby/pull/47867) - Direct routing for IPv6 networks, with `ip6tables` enabled. [moby/moby#47871](https://togithub.com/moby/moby/pull/47871) - Added bridge driver option `com.docker.network.bridge.gateway_mode_ipv6=<nat|routed>`. - The default behavior, `nat`, is unchanged from previous releases running with `ip6tables` enabled. NAT and masquerading rules are set up for each published container port. - When set to `routed`, no NAT or masquerading rules are configured for published ports. This enables direct IPv6 access to the container, if the host's network can route packets for the container's address to the host. Published ports will be opened in the container's firewall. - When a port mapping only applies to `routed` mode, only addresses `0.0.0.0` or `::` are allowed and a host port must not be given. - Note that published container ports, in `nat` or `routed` mode, are accessible from any remote address if routing is set up in the network, unless the Docker host's firewall has additional restrictions. For example: `docker network create --ipv6 -o com.docker.network.bridge.gateway_mode_ipv6=routed mynet`. - The option `com.docker.network.bridge.gateway_mode_ipv4=<nat|routed>` is also available, with the same behavior but for IPv4. - If firewalld is running on the host, Docker creates policy `docker-forwarding` to allow forwarding from any zone to the `docker` zone. This makes it possible to configure a bridge network with a routable IPv6 address, and no NAT or masquerading. [moby/moby#47745](https://togithub.com/moby/moby/pull/47745) - When a port is published with no host port specified, or a host port range is given, the same port will be allocated for IPv4 and IPv6. [moby/moby#47871](https://togithub.com/moby/moby/pull/47871) - For example `-p 80` will result in the same ephemeral port being allocated for `0.0.0.0` and `::`, and `-p 8080-8083:80` will pick the same port from the range for both address families. - Similarly, ports published to specific addresses will be allocated the same port. For example, `-p 127.0.0.1::80 -p '[::1]::80'`. - If no port is available on all required addresses, container creation will fail. - Environment variable `DOCKER_ALLOW_IPV6_ON_IPV4_INTERFACE`, introduced in release 26.1.1, no longer has any effect. [moby/moby#47963](https://togithub.com/moby/moby/pull/47963) - If IPv6 could not be disabled on an interface because of a read-only `/proc/sys/net`, the environment variable allowed the container to start anyway. - In this release, if IPv4 cannot be disabled for an interface, IPv6 can be explicitly enabled for the network simply by using `--ipv6` when creating it. Other workarounds are to configure the OS to disable IPv6 by default on new interfaces, mount `/proc/sys/net` read-write, or use a kernel with no IPv6 support. - For IPv6-enabled bridge networks, do not attempt to replace the bridge's kernel-assigned link local address with `fe80::1`. [moby/moby#47787](https://togithub.com/moby/moby/pull/47787) ##### Removed - Deprecate experimental GraphDriver plugins. [moby/moby#48050](https://togithub.com/moby/moby/pull/48050), [docker/cli#5172](https://togithub.com/docker/cli/pull/5172) - pkg/archive: deprecate `NewTempArchive` and `TempArchive`. These types were only used in tests and will be removed in the next release. [moby/moby#48002](https://togithub.com/moby/moby/pull/48002) - pkg/archive: deprecate `CanonicalTarNameForPath` [moby/moby#48001](https://togithub.com/moby/moby/pull/48001) - Deprecate pkg/dmesg. This package was no longer used, and will be removed in the next release. [moby/moby#47999](https://togithub.com/moby/moby/pull/47999) - Deprecate `pkg/stringid.ValidateID` and `pkg/stringid.IsShortID` [moby/moby#47995](https://togithub.com/moby/moby/pull/47995) - runconfig: deprecate `SetDefaultNetModeIfBlank` and move `ContainerConfigWrapper` to `api/types/container` [moby/moby#48007](https://togithub.com/moby/moby/pull/48007) - runconfig: deprecate `DefaultDaemonNetworkMode` and move to `daemon/network` [moby/moby#48008](https://togithub.com/moby/moby/pull/48008) - runconfig: deprecate `opts.ConvertKVStringsToMap`. This utility is no longer used, and will be removed in the next release. [moby/moby#48016](https://togithub.com/moby/moby/pull/48016) - runconfig: deprecate `IsPreDefinedNetwork`. [moby/moby#48011](https://togithub.com/moby/moby/pull/48011) ##### API - containerd image store: `POST /images/{name}/push` now supports a `platform` parameter (JSON encoded OCI Platform type) that allows selecting a specific platform-manifest from the multi-platform image. This is experimental and may change in future API versions. [moby/moby#47679](https://togithub.com/moby/moby/pull/47679) - `POST /services/create` and `POST /services/{id}/update` now support `OomScoreAdj`. [moby/moby#47950](https://togithub.com/moby/moby/pull/47950) - `ContainerList` api returns container annotations. [moby/moby#47866](https://togithub.com/moby/moby/pull/47866) - `POST /containers/create` and `POST /services/create` now take `Options` as part of `HostConfig.Mounts.TmpfsOptions` allowing to set options for tmpfs mounts. [moby/moby#46809](https://togithub.com/moby/moby/pull/46809) - The `Healthcheck.StartInterval` property is now correctly ignored when updating a Swarm service using API versions less than v1.44. [moby/moby#47991](https://togithub.com/moby/moby/pull/47991) - `GET /events` now supports image `create` event that is emitted when a new image is built regardless if it was tagged or not. [moby/moby#47929](https://togithub.com/moby/moby/pull/47929) - `GET /info` now includes a `Containerd` field containing information about the location of the containerd API socket and containerd namespaces used by the daemon to run containers and plugins. [moby/moby#47239](https://togithub.com/moby/moby/pull/47239) - Deprecate non-standard (config) fields in image inspect output. The `Config` field returned by this endpoint (used for `docker image inspect`) returned additional fields that are not part of the image's configuration and not part of the [Docker Image Spec] and the [OCI Image Spec]. These fields are never set (and always return the default value for the type), but are not omitted in the response when left empty. As these fields were not intended to be part of the image configuration response, they are deprecated, and will be removed in the future API versions. - Deprecate the daemon flag `--api-cors-header` and the corresponding `daemon.json` configuration option. These will be removed in the next major release. [moby/moby#45313](https://togithub.com/moby/moby/pull/45313) The following deprecated fields are currently included in the API response, but are not part of the underlying image's `Config`: [moby/moby#47941](https://togithub.com/moby/moby/pull/47941) - `Hostname` - `Domainname` - `AttachStdin` - `AttachStdout` - `AttachStderr` - `Tty` - `OpenStdin` - `StdinOnce` - `Image` - `NetworkDisabled` (already omitted unless set) - `MacAddress` (already omitted unless set) - `StopTimeout` (already omitted unless set) ##### Go SDK changes - Client API callback for the following functions now require a context parameter. [moby/moby#47536](https://togithub.com/moby/moby/pull/47536) - `client.RequestPrivilegeFunc` - `client.ImageSearchOptions.AcceptPermissionsFunc` - `image.ImportOptions.PrivilegeFunc` - Remove deprecated aliases for Image types. [moby/moby#47900](https://togithub.com/moby/moby/pull/47900) - `ImageImportOptions` - `ImageCreateOptions` - `ImagePullOptions` - `ImagePushOptions` - `ImageListOptions` - `ImageRemoveOptions` - Introduce `Ulimit` type alias for `github.com/docker/go-units.Ulimit`. The `Ulimit` type as used in the API is defined in a Go module that will transition to a new location in future. A type alias is added to reduce the friction that comes with moving the type to a new location. The alias makes sure that existing code continues to work, but its definition may change in future. Users are recommended to use this alias instead of the `units.Ulimit` directly. [moby/moby#48023](https://togithub.com/moby/moby/pull/48023) - Move and rename types, changing their import paths and exported names. [moby/moby#47936](https://togithub.com/moby/moby/pull/47936), [moby/moby#47873](https://togithub.com/moby/moby/pull/47873), [moby/moby#47887](https://togithub.com/moby/moby/pull/47887), [moby/moby#47882](https://togithub.com/moby/moby/pull/47882), [moby/moby#47921](https://togithub.com/moby/moby/pull/47921), [moby/moby#48040](https://togithub.com/moby/moby/pull/48040): - Move the following types to `api/types/container`: - `BlkioStatEntry` - `BlkioStats` - `CPUStats` - `CPUUsage` - `ContainerExecInspect` - `ContainerPathStat` - `ContainerStats` - `ContainersPruneReport` - `CopyToContainerOptions` - `ExecConfig` - `ExecStartCheck` - `MemoryStats` - `NetworkStats` - `PidsStats` - `StatsJSON` - `Stats` - `StorageStats` - `ThrottlingData` - Move the following types to `api/types/image`: - `ImagesPruneReport` - `ImageImportSource` - `ImageLoadResponse` - Move the `ExecStartOptions` type to `api/types/backend`. - Move the `VolumesPruneReport` type to `api/types/volume`. - Move the `EventsOptions` type to `api/types/events`. - Move the `ImageSearchOptions` type to `api/types/registry`. - Drop `Network` prefix and move the following types to `api/types/network`: - `NetworkCreateResponse` - `NetworkConnect` - `NetworkDisconnect` - `NetworkInspectOptions` - `EndpointResource` - `NetworkListOptions` - `NetworkCreateOptions` - `NetworkCreateRequest` - `NetworksPruneReport` - Move `NetworkResource` to `api/types/network`. ##### Packaging updates - Update Buildx to [v0.15.1](https://togithub.com/docker/buildx/releases/tag/v0.15.1). [docker/docker-ce-packaging#1029](https://togithub.com/docker/docker-ce-packaging/pull/1029) - Update BuildKit to [v0.14.1](https://togithub.com/moby/buildkit/releases/tag/v0.14.1). [moby/moby#48028](https://togithub.com/moby/moby/pull/48028) - Update runc to [v1.1.13](https://togithub.com/opencontainers/runc/releases/tag/v1.1.13) [moby/moby#47976](https://togithub.com/moby/moby/pull/47976) - Update Compose to [v2.28.1](https://togithub.com/docker/compose/releases/tag/v2.28.1). [moby/docker-ce-packaging#1032](https://togithub.com/docker/docker-ce-packaging/pull/1032) [Docker image spec]: https://togithub.com/moby/docker-image-spec/blob/v1.3.1/specs-go/v1/image.go#L19-L32 [OCI Image Spec]: https://togithub.com/opencontainers/image-spec/blob/v1.1.0/specs-go/v1/config.go#L24-L62 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/earthly/dind).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

cpuguy83 mentioned this pull request Feb 21, 2023

Docker daemon is leaking file descriptors (with reproduction) #37182

Closed

cpuguy83 changed the title ~~Attach fd leak~~ Fix attach goroutine/fd leak when no I/O is ready Feb 21, 2023

cpuguy83 force-pushed the attach_fd_leak branch 4 times, most recently from 19e39b2 to 53b4d3d Compare February 22, 2023 01:54

cpuguy83 commented Feb 23, 2023

View reviewed changes

api/server/router/container/container_routes.go Show resolved Hide resolved

thaJeztah added status/2-code-review kind/bugfix PR's that fix bugs labels Feb 27, 2023

corhere reviewed Feb 27, 2023

View reviewed changes

api/server/router/container/notify_linux.go Outdated Show resolved Hide resolved

thaJeztah reviewed Feb 28, 2023

View reviewed changes

container/stream/attach_test.go Outdated Show resolved Hide resolved

cpuguy83 mentioned this pull request May 15, 2023

fix attach cause goroutine leak containerd/containerd#8447

Closed

cpuguy83 force-pushed the attach_fd_leak branch from 113c61c to 6381bcf Compare July 5, 2023 22:59

corhere reviewed Jul 6, 2023

View reviewed changes

api/server/router/container/notify_unsupported.go Outdated Show resolved Hide resolved

api/server/router/container/notify_linux.go Outdated Show resolved Hide resolved

cpuguy83 force-pushed the attach_fd_leak branch from 6381bcf to 287892b Compare July 6, 2023 00:13

corhere approved these changes Jul 6, 2023

View reviewed changes

cpuguy83 force-pushed the attach_fd_leak branch from 287892b to ffb951d Compare March 21, 2024 19:00

cpuguy83 force-pushed the attach_fd_leak branch 2 times, most recently from 00b1448 to f9468e1 Compare March 21, 2024 19:23

neersighted approved these changes Mar 21, 2024

View reviewed changes

daemon/attach.go Outdated Show resolved Hide resolved

api/server/router/container/notify_unsupported.go Outdated Show resolved Hide resolved

cpuguy83 force-pushed the attach_fd_leak branch from f9468e1 to 39fce09 Compare March 25, 2024 23:00

cpuguy83 force-pushed the attach_fd_leak branch from 59e015f to b6ae2be Compare May 16, 2024 16:50

cpuguy83 force-pushed the attach_fd_leak branch from b6ae2be to a223d3e Compare May 16, 2024 17:43

cpuguy83 force-pushed the attach_fd_leak branch 2 times, most recently from 2df3ff1 to ac05457 Compare May 23, 2024 18:13

cpuguy83 force-pushed the attach_fd_leak branch 4 times, most recently from 2fb7062 to e87ce9d Compare May 24, 2024 21:31

cpuguy83 added 2 commits May 24, 2024 21:32

cpuguy83 force-pushed the attach_fd_leak branch from e87ce9d to 4619b14 Compare May 24, 2024 21:32

vvoland reviewed Jun 5, 2024

View reviewed changes

vvoland approved these changes Jun 17, 2024

View reviewed changes

vvoland added this to the 27.0.0 milestone Jun 17, 2024

vvoland added the impact/changelog label Jun 17, 2024

thaJeztah added the area/daemon Core Engine label Jun 17, 2024

thaJeztah merged commit b2441c7 into moby:master Jun 17, 2024

cpuguy83 deleted the attach_fd_leak branch July 25, 2024 06:27

Yinette mentioned this pull request Jul 25, 2024

goroutine/fd leak still present in 27.0.3 #48236

Closed

ningmingxiao mentioned this pull request Jan 15, 2025

cri:fix goroutine leak if container doesn't have stdout and stderr when attach. containerd/containerd#11120

Open

Fix attach goroutine/fd leak when no I/O is ready #45052

Fix attach goroutine/fd leak when no I/O is ready #45052

Uh oh!

Conversation

cpuguy83 commented Feb 21, 2023 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Feb 27, 2023

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpuguy83 commented Feb 27, 2023

Uh oh!

Uh oh!

cpuguy83 commented Mar 3, 2023

Uh oh!

corhere commented Mar 31, 2023

Uh oh!

cpuguy83 commented Jul 5, 2023

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Mar 21, 2024

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Mar 26, 2024

Uh oh!

cpuguy83 commented May 16, 2024

Uh oh!

cpuguy83 commented May 24, 2024

Uh oh!

cpuguy83 commented May 31, 2024

Uh oh!

cpuguy83 commented May 31, 2024

Uh oh!

cpuguy83 commented Jun 4, 2024

Uh oh!

Uh oh!

vvoland Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpuguy83 commented Feb 21, 2023 •

edited by thaJeztah

Loading