-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(storage): fix read-write race in Writer.Write #6817
Conversation
Note that the |
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.
Thanks for catching this and for the proposed fix!
I think the spot you have put go monitorCancel()
is too late. In the old code you linked, we launch monitorCancel
after opening the pipe but before launching the go routine that actually sends a request. This reduces the likelihood that we would send a request after the context has already been cancelled. In this PR that logic happens inside the OpenWriter
call on L193.
I think the answer is to pass monitorCancel
in with the openWriterParams
and then launch it from within the transport-specific clients and then have a callback to set Writer.pw
. I can provide more direction on how to do this if you like, or implement it myself. I'd also like to add your repro as a test -- we have another test that checks for a similar condition here but this one passes because the cancellation happens after the first write I believe.
Huh, I'm unfamiliar with the code underneath I can think of at least two less-invasive ideas that have the effect of avoiding initiating the call:
if err := w.ctx.Err(); err != nil {
return err // short-circuit
}
w.pw, err = w.o.c.tc.OpenWriter(params, opts...)
if err != nil {
return err
}
w.opened = true
go w.monitorCancel() This will prevent the extra call in almost all cases.
|
@dragonsinth Thanks for the suggestions, agreed that those would both be less complicated. I think your option (1) seems like a good idea; option (2) seems like a little more complexity if the pipe has already been opened and we have to do cleanup that currently occurs in |
3e1849e
to
09f4b22
Compare
Sweet, updated this PR. |
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.
Looks good to me. I'll add a test based on your example in the issue in a separate PR.
Awesome, thanks @tritone |
Adds a test to detect a potential race condition that can arise if the context is cancelled before writing to GCS. We already have a test that detects a similar condition after we have already started writing the object, but this test looks at the context being cancelled before any writes occur. When run with -race, this test fails in storage/v1.27.0, but it passes when run with the fix in googleapis#6817. Updates googleapis#6816
Adds a test to detect a potential race condition that can arise if the context is cancelled before writing to GCS. We already have a test that detects a similar condition after we have already started writing the object, but this test looks at the context being cancelled before any writes occur. When run with -race, this test fails in storage/v1.27.0, but it passes when run with the fix in #6817. Updates #6816
#6816
w.monitorCancel()
->w.CloseWithError()
reads the values ofw.opened
andw.pw
concurrently with the calling routine initializing them; if the incoming context is cancelled before or during initialization.The fix is simply to deploy starting the monitor until all fields are initialized. The older
open()
code from previous releases did this in the right order:https://github.com/googleapis/google-cloud-go/blob/storage/v1.24.0/storage/writer.go#L130-L134