Repository navigation
Close kqueue sockets before CxPlatSocketDelete returns - #6421
Open
Joel Kiptoo (Kiptoo-Deus) wants to merge 1 commit into
Open
Joel Kiptoo (Kiptoo-Deus) wants to merge 1 commit into
Joel Kiptoo (Kiptoo-Deus) wants to merge 1 commit into
Conversation
On macOS, CxPlatSocketContextUninitialize only queued the close of the socket to the event queue worker, so CxPlatSocketDelete returned while the port was still bound. Binding the same port right after deleting a listener's socket could then fail with QUIC_STATUS_ADDRESS_IN_USE. Close the socket right after removing its read filter. Events the worker has already dequeued are dropped by the upcall rundown, and the rest of the cleanup stays on the event queue. Fixes part 1 of microsoft#6374 Co-authored-by: WilliamSmithEdward <116382973+WilliamSmithEdward@users.noreply.github.com>
This branch has not been deployed
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.
Description
Fixes the macOS part (cause 1) of #6374. The binding that accepted connections hold on to (cause 2) is not changed here.
On the kqueue datapath,
CxPlatSocketContextUninitializeremoves the read filter and queuesShutdownSqe, and theclose()happens later inCxPlatSocketContextUninitializeCompleteon the event queue worker. SoCxPlatSocketDeletereturns while the port is still bound, and binding the same port straight away (for example, starting a new listener afterListenerClose) can fail withQUIC_STATUS_ADDRESS_IN_USE.This closes the socket in
CxPlatSocketContextUninitialize, right after the read filter is removed, and setsSocketFdtoINVALID_SOCKETsoCxPlatSocketContextUninitializeCompleteskips its close. This is safe because:CxPlatRundownReleaseAndWait(&UpcallRundown)has already completed, so any event the worker has dequeued for this socket failsCxPlatRundownAcquireinCxPlatSocketContextIoEventCompleteand doesn't touch the fd.EVFILT_WRITEfilter registered for pending sends.ShutdownSqe.This follows the patch proposed in the issue by William Smith (@WilliamSmithEdward), who is credited as co-author.
The epoll datapath closes its sockets the same deferred way, but with
SO_REUSEPORTon multi-partition server sockets it doesn't show up, and I can't test Linux changes here, so it is left as is.Testing
Added
DataPathTest.UdpRebindSamePortAfterDelete(Windows and macOS only, since epoll still defers the close): bind a UDP socket on the unspecified address, delete it, and bind the same port again 100 times.On macOS arm64 (Release, quictls):
msquicplatformtest --gtest_filter=DataPath*: everything passes exceptXdpMapMode_ZeroConfigUsesNormalPathandXdpMapMode_InitFailsWithoutRawDatapath, which fail on macOS without this change too.msquictest --gtest_filter=*Listener*:*Basic*:*Handshake*:*Rebind*:*Shutdown*: 1127 of 1128 passed. The one failure was a singleMisc/WithAbortiveArgs.AbortiveShutdowncase, which fails intermittently (a different case each time) onmainwithout this change as well.Documentation
None.