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

[cpuguy83]

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.

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 #37182

[thaJeztah]

/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.

[cpuguy83]

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

[cpuguy83]

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]

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]

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!

[corhere]

[cpuguy83]

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.

[thaJeztah]

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

[cpuguy83]

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

[cpuguy83]

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]

Ping, does this LGTY?

[cpuguy83]

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]

Ping

[vvoland]

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?

[cpuguy83]

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.

[vvoland]

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.

[vvoland]

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 👍🏻

[cpuguy83]

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.