sshforward: skip conn close on stream CloseSend. #3431
Merged
Conversation
This file contains 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
The GRPC docs on RecvMsg say: > RecvMsg blocks until it receives a message into m or the stream is > done. It returns io.EOF when the client has performed a CloseSend. > On any non-EOF error, the stream is aborted and the error contains > the RPC status. So if EOF is received that just means the client won't be sending anymore data. But it may still be expecting to read data, so we shouldn't close the conn yet. This was encountered in real life when forwarding a docker socket to a container, where it appears that the docker CLI closes its write side of the connection when requesting the stdout/stderr but then expects to read data after that. Signed-off-by: Erik Sipsma <[email protected]>
tonistiigi
approved these changes
Dec 21, 2022
Merged
aaronlehmann
added a commit
to aaronlehmann/buildkit
that referenced
this pull request
Jan 13, 2023
PR moby#3431 caused connections closed on the remote side of a sshforward session to not always result in the local side reading an EOF from the connection. This change restores that behavior by closing the write side of the forwarded connection after reading an EOF from the stream. Since only the write side is being closed, it doesn't prevent the remote side from continuing to read from the connection.
aaronlehmann
added a commit
to aaronlehmann/buildkit
that referenced
this pull request
Jan 13, 2023
PR moby#3431 caused connections closed on the remote side of a sshforward session to not always result in the local side reading an EOF from the connection. This change restores that behavior by closing the write side of the forwarded connection after reading an EOF from the stream. Since only the write side is being closed, it doesn't prevent the remote side from continuing to read from the connection. Signed-off-by: Aaron Lehmann <[email protected]>
aaronlehmann
added a commit
to aaronlehmann/buildkit
that referenced
this pull request
Jan 14, 2023
PR moby#3431 caused connections closed on the remote side of a sshforward session to not always result in the local side reading an EOF from the connection. This change restores that behavior by closing the write side of the forwarded connection after reading an EOF from the stream. Since only the write side is being closed, it doesn't prevent the remote side from continuing to read from the connection. Signed-off-by: Aaron Lehmann <[email protected]>
tonistiigi
pushed a commit
to tonistiigi/buildkit
that referenced
this pull request
Jan 17, 2023
PR moby#3431 caused connections closed on the remote side of a sshforward session to not always result in the local side reading an EOF from the connection. This change restores that behavior by closing the write side of the forwarded connection after reading an EOF from the stream. Since only the write side is being closed, it doesn't prevent the remote side from continuing to read from the connection. Signed-off-by: Aaron Lehmann <[email protected]> (cherry picked from commit 8c978cf)
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.
The GRPC docs on RecvMsg say:
So if EOF is received that just means the client won't be sending anymore data. But it may still be expecting to read data, so we shouldn't close the conn yet.
This was encountered in real life when forwarding a docker socket to a container, where it appears that the docker CLI closes its write side of the connection after requesting the stdout/stderr stream of a container but then expects to read data after that.
Signed-off-by: Erik Sipsma [email protected]
Related issue in dagger: dagger/dagger#4073