[Windows] Stop silently dropping TCP_NODELAY - #3707
Open
jakepetroules wants to merge 1 commit into
Open
Conversation
`BaseSocket.setOption` refuses to set `TCP_NODELAY` on sockets whose protocol family is not IP, because doing so fails on UNIX domain sockets. It determined the family from `try? self.localAddress().protocol`, treating a failure to answer as "not an IP socket" and returning without setting anything. On Windows that discards the option every time. Winsock fails `getsockname` for a socket that has not been bound yet, where POSIX reports the wildcard address, and channel options are applied before the socket is bound or connected. Both `ClientBootstrap` and `ServerBootstrap` enable `TCP_NODELAY` by default, so in practice Nagle's algorithm stayed on for every NIO TCP channel on Windows. Ask the socket for its address family instead of deriving it from an address it may not have yet: Winsock reports it in the `WSAPROTOCOL_INFOW` returned by `SO_PROTOCOL_INFOW`. Other platforms keep using the local address exactly as before. This fixes `ChannelTests.testTCP_NODELAYisOnByDefaultForInetSockets` and `testTCP_NODELAYisOnByDefaultForInet6Sockets`, which observed the option reading back as 0 on Windows.
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:
BaseSocket.setOptiondoes not setTCP_NODELAYon a socket that is not an IP socket, because that call fails on UNIX domain sockets. To find the family, it readtry? self.localAddress().protocol. If that failed, it treated the socket as "not IP" and set nothing.On Windows this dropped the option every time:
getsocknamefor a socket that is not bound yet, solocalAddress()throws.ClientBootstrapandServerBootstrapboth enableTCP_NODELAYby default.So Nagle's algorithm stayed on for every NIO TCP channel on Windows.
Measured on a Windows ARM64 machine before this change:
After this change, the read back value is 1.
Modifications:
Ask the socket for its address family, instead of reading an address it may not have yet. Winsock returns the family in
WSAPROTOCOL_INFOW, which comes fromSO_PROTOCOL_INFOW. Other platforms still use the local address, so their behaviour does not change.Result:
TCP_NODELAYis now really applied on Windows.This fixes
ChannelTests.testTCP_NODELAYisOnByDefaultForInetSocketsandtestTCP_NODELAYisOnByDefaultForInet6Sockets. Please note that those tests cannot show it in this PR, becauseChannelTestsis still skipped on Windows. Another PR enables that suite, and with both PRs the two tests pass. The measurement above is the evidence for this PR on its own.Tested on a Windows ARM64 machine (Swift 6.3.2): builds on its own, and part of a larger branch where a full
swift testpasses (2360 tests, 0 failures).NIOPosixstill builds on macOS.