Is notify_closing after close() allowed? Fix or document

(Despite my suspiciously LLMesque formatting in places, this was all written by a human.)

Currently, the docs for notify_closing say:

So to close something properly, you usually want to do these steps in order:

1. Explicitly mark the object as closed, so that any new attempts to use it will abort before they start.
2. Call notify_closing to wake up any already-existing users.
3. Actually close the object.

It’s also possible to do them in a different order if that’s more convenient, but only if you make sure not to have any checkpoints in between the steps.

This issue is about that sentence at the end, specifically swapping items 2 and 3: if you close an object before you call notify_closing, is that OK (so long as you do both without a checkpoint in between)?

This came up in pull requests #3503 and #3519, which fix #3500; Part of that issue is notify_closing failing (even if called in the normal order) in an obscure race condition in guest mode. This issue is more about notify_closing (in the wrong order) just in regular non-guest mode usage.

Very short summary

It turns out that if you close the object before notify_closing then that will fail on macOS/BSD (due to a recently-introduced and easily fixable bug), on Windows (due to a much trickier but probably fixable bug) and appear to succeed on Linux but with potentially spurious wait wakes later if the handle had been duped (and that is impossible to fix).

If you just want to know what to do about that, you can skip to "my suggestions" at the bottom!

Background: what does notify_closing do?

It isn't obvious why this is needed: ideally, closing a socket would cause the underlying OS wait (epoll/kqueue/IOCP) to wake up any readable/writable wait with some error that indicates that the socket was closed, which we could translate to ClosedResourceError at the point we receive it. This doesn't work for reasons that are different on the different IO managers.

kqueue When you close a file descriptor, it automatically removes the event subscription from the kqueue. However, the removal is silent: readable/writable waiters aren't notified in any way (there's no "FD was closed" event sent to them), so Trio wait_readable / wait_writable would wait forever. The notify_closing call does two things to fix this:

• Removes the event from the poll set (_kqueue.control([kevent(fd, filter, KQ_EV_DELETE)], 0)). As I just said, this happens anyway on close, so this is totally redundant.
• For any waiters (callers of wait_readable / wait_writable) on that socket, wake immediately with reschedule(ClosedResourceError).

epoll Like kqueue, when you close a file descriptor, it removes the event subscription, but this doesn't happen if the socket is duped (see below). Even if the subscription is removed, waiters aren't notified. notify_closing does the same two things:

• Removes event from poll set (_epoll.unregister(fd)) - this is not redundant, as the FD could have been duped.
• Wakes waiters with reschedule(ClosedResourceError).

IOCP Unlike the other two, closing the socket actually does notify the waiters (with AFD_POLL_LOCAL_CLOSE, which Trio could translate into ClosedResourceError, but it currently doesn't). If it were no more complicated than that the IOCP IO manager wouldn't need a notify_closing call at all! But Windows has the same issue as epoll that if the socket was duped then operations continue, so notify_closing tidies everything up manually:

• The underlying OS request for waiting is cancelled (IoCancelEx)
• Wakes waiters with reschedule(ClosedResourceError).
• Removes the waiters from Trio's little struct that says what to do when the wait operation completes (which it will, later, with a "cancelled" status).

More background: the issue with dup

If you duplicate the socket handle, like this (taken from a comment in _io_epoll.py):

fd1 = open(...)
epoll_ctl(EPOLL_CTL_ADD, fd1, ...)
fd2 = dup(fd1)
close(fd1)

Then fd1 is not automatically removed from epoll after all. It continues to report events on the old fd1, even though it's closed. Once fd1 is closed, you can't even remove it manually, because it's invalid to call epoll_ctl on a closed fd! Even worse, if a new file/socket gets opened on that FD number, then events for the old socket object will continue to appear with no way to distinguish whether they came from the old or new one!

(The comments in the IO manager worry about a busy loop: If a socket is readable/writable but we never remove it from the poll set because we've lost track of it, and because we can't anyway, then get_events() would return immediately every time. The epoll code handles this by only issuing one-shot subscriptions so we can't get more than one spurious wake up from an event we've lost track of.)

The same issue occurs on Windows: closing a socket doesn't signal readable/writable waiters if it has been duped. But it's not as bad because if the event triggers later then it points at the Trio's struct with waiters (which were removed by notify_closing) so it can't wake the wrong thing, and it can still be cancelled because cancellation doesn't use the socket handle. (And it can't busy-wait because it's always a one-shot event anyway.)

What happens if we call close before notify_closing?

Here's what currently happens if you call close then notify_closing, as supposedly permitted by the above text:

• kqueue (macOS/BSD): It doesn't work: wait_readable / wait_writable just continue to wait indefinitely.
• IOCP (Windows): notify_closing raises OSError(10038, 'An operation was attempted on something that is not a socket'). Regardless of this (i.e. with and without a simple fix for it), wait_readable / wait_writable do wake, but just as a regular return (not ClosedResourceError).
• epoll (Linux): Works correctly: notify_closing returns normally, and waiters raise ClosedResourceError.

However, this all assumes that you haven't called dup on the socket. Part of the point of notify_closing (in fact the only point of it on Windows) is to handle that case. If you do:

• kqueue (macOS/BSD): (Assuming we fix the issue in kqueue notify_closing mentioned above) This works fine: kqueue already removed the event based on the file descriptor, regardless of whether it was duped, so when notify_closing attempts to remove it gets an error but ignores it, and then wait_readable / wait_writable wake and raise ClosedResourceError.
• IOCP (Windows): (Assuming we fix the issue in IOCP notify_closing mentioned above (properly, not using my non-fix suggested below)) Cancellation of the outstanding wait works, and when we get the cancellation notification it points at a struct with no waiters left registered in it.
• epoll (Linux): The removal of the pollers doesn't work because the file handle is no longer valid, but waiters do wake and raise ClosedResourceError.

Solving the issue with kqueue notify_closing after close

This is a recent bug introduced by #3503 to fix #3500 (which fixes a race condition in guest mode which happens even when notify_closing is correctly called before close). If removing the registration from the kqueue fails then, with that change, we bail out from doing anything else. But, if the socket has been closed, then removing from the kqueue has already automatically happened, so it will fail in notify_closing.

I mentioned two fixes in this comment for the notify_closing race condition. If we use the other one (if we get an error removing the kqueue registration, continue regardless, but make sure we don't fail later in process_events due to a missing _registered entry) then that still fixes the race condition and also allows notify_closing after close. (When implementing "allowed missing _registered entries" as suggested here, we'll need to include these.) (As a bonus, when this race condition occurs, waiters will get ClosedResourceError instead of a plain return.)

Solving the exception from IOCP notify_closing after close

The Windows IOCP IO manager doesn't use regular documented IOCP operations to read and write (because they actually perform the read/write, not just wait for it to be possible synchronously). Instead they use some internal undocumented (but stable) interface called AFD. This requires translating the socket handle into an internal handle, which is almost always the same but may not under conditions that happen just enough not to ignore.

The first thing notify_closing() does is translate the normal handle into the internal one, and uses that to determine which overlapped operation to cancel. If the cancellation fails then that's ignored (which it should be - it could happen due to the same race in #3500). But, first, if the socket is closed then the lookup of the underlying handle fails. This is the cause of the issue in Windows.

I haven't really looked into the best way to solve this yet. It seems that Trio is odd in repeatedly looking up the underlying handle from the socket handle - other frameworks cache this. (Trio does at least cache it for issuing a IOCP cancel due to a Trio task cancellation.) One option for fixing this would be to do that. But I don't really understand why it needs to look up the underlying handle anyway at this point anyway; it's not needed for the cancellation, only to lookup who the waiters are. Why not just key _afd_waiters on the user level handle? I guess there's a worry that multiple socket handles could point to the same AFD handle but I don't see how that's possible (but I haven't tried to look into it).

One option suggested by an LLM is, as a fallback, to just assume the underlying handle is the same as the one provided by the caller, which does work in most cases. But I think that's best avoided.

Another option is to just silently return if we can't look up the underlying handle, since notify_closing isn't even needed unless the socket was duped. This would also require turning AFD_POLL_LOCAL_CLOSE into ClosedResourceError, which it currently isn't (because currently notify_closing cancels the requests anyway). This seems like the best option to me.

Solving the failed unregistration in epoll after dup

For epoll, the big reason to use notify_closing is that, after the socket is closed, there's no way to remove waiters from the poll set.

So a version of notify_closing that works even if called after close is fundamentally impossible. This is a dead end.

The only solution here is to update the documentation to say that this isn't allowed.

I suppose it's worth noting that the consequences are not that catestrophic: the result is just that an unrelated socket might get a spurious wake up for wait_readable / wake_writable, which will probably be followed by a failed sync call and a retry on the wait.

Normal return (no exception) from wait readable/writable on a closed socket

While looking into this I noticed a slightly tangential issue.

Regardless of what we notice or fix relating to notify_closing, it's always possible for wait_readable / wait_writable to simply return (rather than raise ClosedResourceError) after the socket has been closed, even if following the correct notify_closing -> close pattern.

That's because this might happen:

1. The IO wait (i.e. get_events()) completes, and it includes a regular readable/writable notification.
2. Two tasks are scheduled (in the runq): task 1 for some other reason, task 2 because a socket is now readable.
3. The tasks happen to run in this order:
   1. Task 1 resumes and closes the socket (with notify_closing then close). This does not reschedule task 2 because it's no longer waiting from the IO manager's perspective.
   2. Task 2 resumes with a normal return from wait_readable.

I notice that the underlying trio.socket operations do not handle this. Most of them use _make_simple_sock_method_wrapper, which calls _nonblocking_helper, which follows a call to wait_fn (i.e. wait_readable / wait_writable) immediately with a call to fn (e.g. socket.recv). Internally, the Python socket object has set its fd to -1, so this fails with OSError ([Errno 9] EBADF on Linux, [WinError 10038] WSAENOTSOCK on Windows).

It is checked by SocketStream, which uses _translate_socket_errors_to_stream_errors to translate EBADF / ENOTSOCK to ClosedResourceError.

Personally, I'd consider this a bug. If it happened every time, that would be excusable as a design decision, but having a different exception only under a very narrow race seems hard to excuse to me. There are two options for fixing it:

• The wait functions do a last moment check just after await wait_task_rescheduled() to see whether the socket has been closed. This would be ideal but they don't have neccesarily access to the socket object (the parameter can be a socket or a FD int) so they can't necessarily do a quick fd == -1 check.
• Have the socket operations in Trio socket do a check just after the wait function before calling the sync version (or translate errors from the sync call like SocketStream does, but not BrokenResourceError as that would be a significant breaking change). And add a warning to the documentation of the wait functions that they might fail to raise ClosedResourceError for closed files.

My suggestions

• In the documentation for notify_closing:
  • State that it must be called before the socket is closed, rather than just "you usually want to".
  • Say that if the socket is closed first then Trio will make a best effort to handle the situation but it's not guaranteed to work.
• kqueue IO manager (macOS/BSD):
  • Use the alternative fix for #3500 notify_closing as discussed above (and, in guest mode, add to the set of recently-removed _registered entries). I'll do this as part of #3519.
• IOCP IO manager (Windows):
  • When processing events, translate AFD_POLL_LOCAL_CLOSE into ClosedResourceError.
  • In notify_closing, if there's an error looking up the handle, just return early and hope that socket closure notifies the IOCP (if the socket wasn't duped) so, with the previous point done, waiters get the correct result.
  • I think these are both fairly low priority since this has been like this for a long time - and we should be recommending against this usage anyway.
• All IO managers:
  • In wait_readable and wait_writable, if the parameter is not an int then do a last moment check to see if the FD has become -1, and if so then raise ClosedResourceError.
• epoll IO manager (Linux):
  • Nothing to do because the dup issue is fundamentally unfixable with close before notify_closing