multiplexer: don't fail if Close() is called concurrently to Read(), Write()#632
Merged
djs55 merged 2 commits intomoby:masterfrom Apr 11, 2023
Merged
multiplexer: don't fail if Close() is called concurrently to Read(), Write()#632djs55 merged 2 commits intomoby:masterfrom
djs55 merged 2 commits intomoby:masterfrom
Conversation
Instead send the errors back to the main goroutine. Signed-off-by: David Scott <dave@recoil.org>
A test run demonstrated a two-way Close ``` [2023-04-04T05:48:03.965050200Z][com.docker.backend.exe][I] recv 3 Close [2023-04-04T05:48:03.965050200Z][com.docker.backend.exe][I] send 3 Close ``` at this point the channel refcount is 0 and the channel considered closed ``` [2023-04-04T05:48:03.965050200Z][com.docker.backend.exe][I] close 3 -> UDP:127.0.0.1:5201 ``` however an in-progress Read call read some buffered data and sent a window advertisement: ``` [2023-04-04T05:48:03.965050200Z][com.docker.backend.exe][I] recv 3 Window 935323206 ``` which broke the multiplexer with ``` [2023-04-04T05:48:03.918050500Z][com.docker.backend.exe][I] Multiplexer main loop failed with Unknown channel id 3 Window 935323206 ``` Previously it was illegal to send anything after a Close(). However it's legal for clients to call Close() in goroutines while other goroutines are calling Read() or Write(). Each send call competes for the writeMutex, which means it's possible for the messages to be sent out-of-order. Since we have 2**32 channel numbers there should not be any confusion if we simply expect the occasional out-of-order packet after a Close and drop it. Signed-off-by: David Scott <dave@recoil.org>
p1-0tr
approved these changes
Apr 11, 2023
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
2 participants
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.
It's legal for a single
net.Connto be shared between goroutines, for example callingClose()on<- ctx.Done()while calls are blocked inReadandWrite.The multiplexer does not impose an ordering in
send: each concurrent call simply competes for thewriteMutex.Therefore sequences we considered invalid are actually possible:
Close(id)thenWindow(id)Close(id)thenWrite(id)Close(id)thenShutdown(id)Previously we detected these invalid sequences and shutdown the whole multiplexer. The effect would be to completely disable port forwarding.
Instead we drop these out-of-order packets and log (just in case it's interesting). There is a very small risk that an old out-of-order packet could be mixed up with a new packet if the channel ID is reused, but we have a space of 2**32 of those so re-use is unlikely.