-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix interrupted pending writes on socket write shutdown from eager close #162
Conversation
PTAL @praveenkumar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, though having a bit more details in the commit log would not hurt :)
If I understand correctly, when one of the forward() functions calls Close(), this prevents some writes in the other forward() from completing?
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cfergeau, n1hility The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Some channel/connection implementations may signal EOF to parallel readers before tasks related to the CloseWrite (shutdown) have completed progressing. This creates the potential for a race with a parallel Close(), leading to a premature abort of certain activies (cancelling the send of buffered data). This change ensures that the two goroutines copying each direction of the stream wait until CloseWrite has completed in both directions before fully closing. Signed-off-by: Jason T. Greene <[email protected]>
7bab90c
to
db11bd9
Compare
@cfergeau thanks for looking, and good point, I added a more detailed description to the commit and PR description. One other thing I wanted to ask about was to pull this in podman we will need a gvproxy release. Glancing at the commits since 0.40 it seems all the changes are stability related, with the exception of a few extra hooks. Do you feel the other changes on the current branch are good to release? Are you currently coordinating releases? Thanks! |
I agree there's nothing particularly worrying in the recent changes. Would be nice to have some fix for #161 while at it, but I have no idea how much work this involves, gbraad was looking at it.
gbraad did the last release, but we definitely could have a release soon, it's been a while since the last one even if there aren't many changes. |
I created #163 so that this release discussion does not get lost. |
Addresses the underlying ssh forwarding bug behind containers/podman#16656
Some channel/connection implementations may signal EOF to parallel
readers before tasks related to the CloseWrite (shutdown) have
completed progressing. This creates the potential for a race with
a parallel Close(), leading to a premature abort of certain activies
(cancelling the send of buffered data).
This change ensures that the two goroutines copying each direction
of the stream wait until CloseWrite has completed in both directions
before fully closing.