[Windows] Read SO_ERROR as a winsock error - #3710
Open
jakepetroules wants to merge 3 commits into
Open
Conversation
`CNIOWindows_sendmmsg` was a stub that asserted and then called `abort()`, so any code path reaching it took the whole process down. That is the datagram write path whenever more than one write is pending, which made `DatagramChannelTests` and `RawSocketBootstrapTests` unrunnable: both aborted partway through, taking the rest of the test process with them. Winsock has no `sendmmsg`, so emulate it by sending the messages one at a time, which is what the Darwin shim does for the same reason. Implement it in Swift alongside the existing `sendmsg`, rather than filling in the C shim, since everything needed is callable directly from Swift; the now-unused C stub and its declaration are removed. The `WSASendMsg` extension function is resolved once per batch rather than once per message, so the lookup is hoisted into a helper shared with `sendmsg`. The error semantics match the other implementations: a failure on the first message is reported to the caller, with `WSAEWOULDBLOCK` reported as `.wouldBlock` the way the POSIX implementation's `syscall(blocking: true)` wrapper does, while a failure after some messages have been sent is reported as a short send for the caller to retry. This does not yet make either test suite pass, so no skips are removed here. Both now get further before failing for unrelated reasons: the datagram tests reach a `fatalError` in `PendingDatagramWritesManager`, which reads `errnoCode` on an `IOError` that carries a winsock-domain code, and the raw socket tests fail with "Already closed". Crucially, neither aborts the test process any more.
Depends on apple#3699. `IOError.errnoCode` trapped with `fatalError("IOError domain is not errno")` whenever the error carried a winsock- or windows-domain code, which NIO's own error classification then walked straight into. `PendingDatagramWritesManager` compares against `EMSGSIZE` and `EHOSTUNREACH` to decide whether a datagram write error is recoverable, and `BaseSocket` compares against `EAFNOSUPPORT` when it turns off `IPV6_V6ONLY` -- the latter on the path of every IPv6 socket NIO creates. Winsock's error codes mean the same things as their errno counterparts, so translate them rather than trapping: `WSAEMSGSIZE` reports `EMSGSIZE`, and so on. Error handling written against errno then keeps working on Windows, which also matters for the tests, where there are ~38 comparisons against errno constants. Codes with no errno counterpart, and windows-domain codes, are reported unchanged rather than trapping. That is unambiguous: Windows CRT errno values never exceed 140, while winsock and Win32 codes are far larger, so such a value cannot be mistaken for an errno. Note that the Windows CRT gives `EWOULDBLOCK` and `EAGAIN` distinct values, unlike the platforms where they are synonyms, so `WSAEWOULDBLOCK` is paired with `EWOULDBLOCK`. With this and apple#3699, `DatagramChannelTests` runs to completion on Windows for the first time (74 tests: 38 pass, 19 fail, 17 skipped) rather than aborting the test process. The remaining failures are unrelated gaps, so the suite stays skipped for now and no skips are removed here.
Depends on apple#3700. `SO_ERROR` reports a Winsock error code on Windows, not an `errno`, but the three places that read it wrapped the value in an errno-domain `IOError`. The codes then made no sense: `ChannelTests.testConnectWithECONNREFUSEDGetsTheRightError` saw 10061 (`WSAECONNREFUSED`) where it expected 107 (`ECONNREFUSED`). `SocketChannel.error()` also compared the raw value against `ECONNREFUSED` and `ENOMEM` to decide whether the error is recoverable, so on Windows a recoverable error was always classified as fatal and reset the channel. Add an `IOError(socketError:reason:)` initialiser that records the value in the domain it actually belongs to, and use it in `Socket.finishConnect`, `BaseSocketChannel` and `SocketChannel.error()`. Classification now goes through `IOError.errnoCode`, which reports the corresponding `errno` on Windows, so the comparisons hold on every platform and the `#if os(Windows)` branch in `shouldCloseOnError` is no longer needed.
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
1 participant
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:
Depends on #3699 and #3700, which are the first two commits on this branch. Please review only the last commit, or wait until those are merged.
On Windows,
SO_ERRORreports a Winsock error code, not an errno. Three places read it and put the value into an errnoIOError. The codes then made no sense.ChannelTests.testConnectWithECONNREFUSEDGetsTheRightErrorsaw 10061 (WSAECONNREFUSED) where it expected 107 (ECONNREFUSED).There is a second effect.
SocketChannel.error()compares that value withECONNREFUSEDandENOMEMto decide if the error can be recovered. On Windows the comparison never matched, so every such error was treated as fatal and reset the channel.Modifications:
IOError(socketError:reason:). It records the value in the domain it belongs to: winsock on Windows, errno elsewhere. Use it inSocket.finishConnect,BaseSocketChannelandSocketChannel.error().IOError.errnoCode. On Windows that reports the matching errno ([Windows] Report the errno equivalent of winsock errors #3700), so the comparisons work on every platform. The#if os(Windows)branch inshouldCloseOnErroris no longer needed.The comment about
ECONNREFUSEDon Linux moves next to the cases it describes, so it is not lost.Result:
Connect failures report the correct error on Windows, and a recoverable socket error is no longer treated as fatal there.
This fixes
ChannelTests.testConnectWithECONNREFUSEDGetsTheRightError. That test runs onceChannelTestsis enabled on Windows, which another PR does.Tested on a Windows ARM64 machine (Swift 6.3.2): builds,
NIOPosixstill builds on macOS, and part of a larger branch where a fullswift testpasses (2360 tests, 0 failures).