[Windows] Remove deregistered sockets from the WSAPoll set before polling - #3706
Open
jakepetroules wants to merge 1 commit into
Open
[Windows] Remove deregistered sockets from the WSAPoll set before polling#3706jakepetroules wants to merge 1 commit into
jakepetroules wants to merge 1 commit into
Conversation
…ling
The WSAPoll selector deferred removal of deregistered sockets from `pollFDs`,
recording the *index* of each entry in `deregisteredFDs` and compacting the
array after delivering events. That had three problems.
The compaction ran only on the `result > 0` path, so a socket deregistered
before a poll that timed out (or failed) kept its entry. Sockets are usually
closed immediately after being deregistered, so the next poll would be handed a
closed handle, earning a `POLLNVAL` and tripping the
`preconditionFailure("Invalid fd supplied.")` in `SelectorEventSet`.
The compaction also removed the entry's registration from `registrations`, which
`Selector.deregister` has already done by that point. That is not merely
redundant: Windows reuses socket handles readily, so if the same handle had been
registered again in the meantime, this deleted the *new* registration and the
new channel silently stopped receiving events.
Finally, deferring by index meant a handle could occupy two live entries at
once -- the old, still-present entry and a new one appended by `register0` --
so a single readiness notification could be delivered twice. The second
delivery finds nothing to read, which trips `assert(readResult == .some)` in
`BaseSocketChannel`.
Tombstone the entry in `deregister0` instead, clearing its handle immediately so
that a re-registration of the same handle cannot be confused with it, and
compact `pollFDs` at the top of `whenReady0`, before polling. Doing it there
covers every path out of the previous call, and guarantees a closed handle is
never handed to `WSAPoll`. `deregisteredFDs` is replaced by a flag, which is
also cheaper than a `Set<Int>`.
Add assertions for the two invariants this relies on: that the wakeup socket
stays at `pollFDs[0]`, and that a socket is not registered twice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
The WSAPoll selector did not remove a deregistered socket from
pollFDsright away. It saved the array index inderegisteredFDs, and compacted the array later, after it delivered events. This caused three problems.1. A closed handle could be polled. The compaction only ran when the poll returned events. If a socket was deregistered and the next poll timed out, its entry stayed in the array. NIO closes a socket right after it deregisters it, so the next
WSAPollreceived a closed handle.WSAPollthen reportsPOLLNVAL, and the selector treats that as fatal inpreconditionFailure("Invalid fd supplied."). #3685 made deregistration really happen during shutdown, so this became easier to hit.2. A live registration could be deleted. The compaction also removed the registration from
registrations, butSelector.deregisterhad already done that. Windows reuses socket handles often. If the same handle was registered again in the meantime, this deleted the new registration, and the new channel stopped receiving events.3. One handle could have two entries. The old entry was still in the array, and
register0could append a second entry for the same handle. One readiness event was then delivered twice. The second delivery finds no data, which tripsassert(readResult == .some)inBaseSocketChannel. I have seen this fail from time to time on Windows.Modifications:
deregister0now clears the handle in the entry immediately. A laterregister0for the same handle cannot be mixed up with it.whenReady0compactspollFDsat the start, before it polls. This covers every path out of the previous call, so a closed handle is never given toWSAPoll.registrations.Selector.deregisterowns that.deregisteredFDsbecomes aBool, which is cheaper than aSet<Int>.pollFDs[0], and a socket is not registered twice.POLLNVALstill traps, on purpose. LikeEBADF, it means NIO lost track of a descriptor. Windows reuses handles, so using such a handle could send I/O to the wrong channel. After this change the deregister-then-close path no longer causesPOLLNVAL, so a trap there now points to a real bug.These files are Windows-only. Other platforms are not affected.
Result:
Tested on a Windows ARM64 machine (Swift 6.3.2). This commit builds on its own. It was also tested inside a larger branch, where a full
swift testpasses (2360 tests, 0 failures) in several runs.About the
assert(readResult == .some)failure: this removes one cause, but maybe not the only one. A listening socket can reportPOLLRDNORM, andacceptcan then returnWSAEWOULDBLOCK, which looks the same. So please read this PR as a fix for the three problems above, not as a proven fix for that failure.