Skip to content

[Windows] Deregister channels while the event loop is closing - #3685

Open
jakepetroules wants to merge 1 commit into
apple:mainfrom
jakepetroules:windows-wsapoll-nval
Open

[Windows] Deregister channels while the event loop is closing#3685
jakepetroules wants to merge 1 commit into
apple:mainfrom
jakepetroules:windows-wsapoll-nval

Conversation

@jakepetroules

Copy link
Copy Markdown
Member

Motivation:

The WSAPoll selector traps with preconditionFailure("Invalid fd supplied.") whenever WSAPoll reports POLLNVAL, and that is reachable from ordinary shutdown, so MultiThreadedEventLoopGroupTests.testShuttingDownFailsRegistration is currently skipped on Windows.

The root cause is not in the selector. SelectableEventLoop.deregister guards on isOpen, which is false both when the loop is closed and when it has merely stopped accepting new registrations. The latter is precisely the state the loop is in during Selector.closeGently, which closes each remaining channel — so those channels skip deregistration entirely and then close their sockets anyway.

The epoll and kqueue backends tolerate that, because there a registration lives in the kernel and is dropped along with the descriptor. The WSAPoll backend keeps its registrations in an array of its own, so it is left holding a stale entry for a closed socket, and the next WSAPoll correctly reports POLLNVAL for it.

Traced on a Windows ARM64 VM by instrumenting the selector and the event loop:

register0 fd=704 idx=1
EL.deregister SKIPPED state=runningButNotAcceptingNewRegistrations
EL.deregister SKIPPED state=runningButNotAcceptingNewRegistrations
POLLNVAL fd=704 i=1 revents=4 events=256 dereg=[] indexes=[704: 1, 712: 2, 728: 3]
Fatal error: Invalid fd supplied.

Modifications:

  • Guard deregister on the selector's lifecycle rather than on isOpen. Deregistering an existing channel while the loop is closing is valid, because the selector itself is still open at that point.
  • Remove the skip on MultiThreadedEventLoopGroupTests.testShuttingDownFailsRegistration.

POLLNVAL deliberately keeps trapping. Like EBADF, which NIO already treats as an unacceptable errno, it means we have lost track of a descriptor; Windows recycles SOCKET handles, so continuing against a possibly-reused handle would misroute I/O to the wrong channel. An earlier version of this patch mapped POLLNVAL to .error instead — that silently retained the stale entry and gave up that guarantee, so it was dropped in favour of fixing the cause.

Note for reviewers: unlike most of the Windows porting patches this touches shared code rather than sitting behind #if os(Windows). The observable behaviour on Linux/Darwin is unchanged, but they will now issue one epoll_ctl(EPOLL_CTL_DEL)/kevent delete per channel during event loop shutdown where previously they issued none.

Result:

The WSAPoll selector no longer traps during event loop shutdown, and testShuttingDownFailsRegistration runs on Windows.

Verified:

  • Windows ARM64 (Swift 6.3.2), full unfiltered swift test: 2354 tests, 0 failures.
  • macOS, full swift test with and without this change: identical results (2451 tests, same 28 pre-existing failures caused by the network mount used for this port, none in NIOPosix).

The WSAPoll selector trapped with `preconditionFailure("Invalid fd supplied.")`
whenever `WSAPoll` reported `POLLNVAL`, and that was reachable from ordinary
shutdown.

`SelectableEventLoop.deregister` guarded on `isOpen`, which is false both when
the loop is closed *and* when it has merely stopped accepting **new**
registrations. The latter is the state the loop is in during
`Selector.closeGently`, which closes each remaining channel -- so those channels
skipped deregistration entirely and then closed their sockets anyway. The epoll
and kqueue backends tolerate that, because there a registration lives in the
kernel and is dropped along with the descriptor. The WSAPoll backend keeps its
registrations in an array of its own, so it was left holding a stale entry for a
closed socket, and the next `WSAPoll` correctly reported `POLLNVAL` for it.

Deregistering existing channels in that state is valid -- the selector itself is
still open -- so guard on the selector's lifecycle instead. `POLLNVAL` keeps
trapping: like `EBADF`, which NIO already treats as unacceptable, it means we
have lost track of a descriptor, and acting on one that may since have been
reused would misroute I/O to the wrong channel.

This unskips `MultiThreadedEventLoopGroupTests.testShuttingDownFailsRegistration`.
@jakepetroules

Copy link
Copy Markdown
Member Author

@weissi / @compnerd may want a look here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant