[Windows] Prevent socket callback from falsely detecting a connection close #578
Conversation
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
|
CC: @compnerd |
compnerd
requested changes
Oct 11, 2021
There was a problem hiding this comment.
I think that #581 might be the change that this is intending to make - which seems like it should be correct. A racy read is possible between WSAEnumNetworkEvents and ioctlsocket. I'm not sure how to create a decent test case for that though.
| _dispatch_muxnote_retain(dmn); | ||
| if (!PostQueuedCompletionStatus(hPort, dwBytesAvailable, | ||
| (ULONG_PTR)DISPATCH_PORT_SOCKET_READ, (LPOVERLAPPED)dmn)) { | ||
| DISPATCH_INTERNAL_CRASH(GetLastError(), | ||
| "PostQueuedCompletionStatus"); | ||
| } | ||
| } | ||
| if (lNetworkEvents & FD_WRITE) { | ||
| if (((lNetworkEvents & FD_WRITE) && dwBytesAvailable > 0) || (lNetworkEvents & FD_CLOSE)) { |
There was a problem hiding this comment.
This doesn't make sense - dwBytesAavailable is guaranteed 1 if only a write event occurred. This condition doesn't seem correct. It should be either a write or a close event occurred I believe.
| // Here and below we have to double-check dwBytesAvailable and make sure we don't | ||
| // wake up read/write handlers with 0 value for live (not closed) socket, because | ||
| // that would force handler to shut down. | ||
| if (((lNetworkEvents & FD_READ) && dwBytesAvailable > 0) || (lNetworkEvents & FD_CLOSE)) { |
There was a problem hiding this comment.
This seems plausible to occur. But FD_READ is already set when FD_CLOSE is set, so this is rather confusing.
|
Closing in favor of #581 |
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.
This fixes a hard-to-reproduce issue we did observe in our live product (powered by Swift on Windows). The application communicates with the backend a lot, and some network requests were randomly timing out. All attempts to reproduce that behavior synthetically failed, but in the end we got some low but stable repro rate and had a chance to debug this.
It appeared that sometimes Dispatch catches
FD_READevent, butioctlsocketsays that we have nothing to read at the moment of event processing. That is perfectly ok, non-zero result is not guaranteed there, as the reading could occur asynchronously.But 0 is also used as a special value in
_dispatch_event_mergeas a marker of a closed socket. Such sockets are exclude from any further handling, making the connection hang.The suggested solution avoids read/write processing with 0 available bytes, except for
FD_CLOSEevent, when we really need to poke all handlers to actually shut down the socket. TheFD_READevent with 0 bytes becomes a no-op and doesn't do anything.We applied this fix locally more than a year ago and since then the connection behavior seems to be stable.