- Notifications
You must be signed in to change notification settings - Fork927
fix: Race when writing to a closed pipe#1916
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cec2de3 to8e61cacCompareTo me it seems like this could actually be a bug in yamux. Fromhttps://pkg.go.dev/io#Pipe:
It doesn't sound like we need to guard Read/Write/Close. And looking at the stack trace, it seems yamux session is doing slice mutations both at https://github.com/hashicorp/yamux/blob/0bc27b27de87381b58e35462a8dcbd69040279c1/stream.go#L201 https://github.com/hashicorp/yamux/blob/0bc27b27de87381b58e35462a8dcbd69040279c1/stream.go#L365 There's one more use of https://github.com/hashicorp/yamux/blob/0bc27b27de87381b58e35462a8dcbd69040279c1/session.go#L642 And |
Yup, I've come to the same conclusion! Thanks for the investigation@mafredri 🥳 |
No prob! And looks like I was wrong about |
@mafredri if I changehttps://github.com/storj/drpc/blob/main/drpcwire/writer.go#L85 to copy the buffer with |
This is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.
@mafredri I changed this to use |
I ran thisa lot without the failure..... seems to work! |
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.
Awesome refactor,net.Pipe made this beautiful!
I think if you can't repro, this is good to 🚀.
Meanwhile, I did make a PR about that one discovery:hashicorp/yamux#100
This is such an intermittent race it's difficult to track,
but regardless this is an improvement to the code.