Clear svr_sock_ before closing it on the accept loop's fatal path - #2560
Merged
Conversation
When accept() failed for a reason the retry branches do not cover, the loop closed svr_sock_ but left the descriptor in the atomic. Two things go wrong from there: - A later stop() reads the stale value and calls shutdown()/close() on it. By then the OS may have reused the descriptor for an unrelated socket (a worker's keep-alive connection, or one the application opened), and that connection is torn down instead. - keep_alive() in the worker threads watches svr_sock_ to notice that the server is going away, so the workers keep waiting on a listening socket that no longer exists. Take the descriptor with exchange(INVALID_SOCKET) before closing it, which is what stop() already does. That also settles the race with a concurrent stop(): whichever side takes the descriptor closes it exactly once, and the other sees INVALID_SOCKET and does nothing.
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.
When
accept()failed for a reason the retry branches do not cover, the loop closedsvr_sock_but left the descriptor in the atomic. Two things go wrong from there:stop()reads the stale value and callsshutdown()/close()on it. By then the OS may have reused the descriptor for an unrelated socket (a worker's keep-alive connection, or one the application opened), and that connection is torn down instead.keep_alive()in the worker threads watchessvr_sock_to notice that the server is going away, so the workers keep waiting on a listening socket that no longer exists.Take the descriptor with
exchange(INVALID_SOCKET)before closing it, which is whatstop()already does. That also settles the race with a concurrentstop(): whichever side takes the descriptor closes it exactly once, and the other seesINVALID_SOCKETand does nothing.There is no portable way to force
accept()into the fatal path from a test, so this comes without one.Supersedes #2555, which reported the same bug and fixes it the same way.