Skip to content

fix(storage): propagate ctx from invoke to grpc upload reqs#11475

Merged
BrennaEpp merged 2 commits into
googleapis:mainfrom
BrennaEpp:fix-ctxpropagation
Jan 27, 2025
Merged

fix(storage): propagate ctx from invoke to grpc upload reqs#11475
BrennaEpp merged 2 commits into
googleapis:mainfrom
BrennaEpp:fix-ctxpropagation

Conversation

@BrennaEpp

@BrennaEpp BrennaEpp commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

The context from run (see invoke.go) is not being used for gRPC uploads.

Rather than keep a source of truth in a writer var (except for the original context), pass through contexts.

The other option is to keep as is and instead do the following. This may be "safer" (at least in terms of, more likely to keep current behaviour) as it is hard to follow exactly how these contexts are behaving currently.

                                 uploadBuff := func(ctx context.Context) error {
					saveCtx := gw.ctx
					gw.ctx = ctx
					obj, err := gw.uploadBuffer(ctx, recvd, offset, doneReading)
					o = obj
					gw.ctx = saveCtx
					return err
				}
  • Note: bucketContext also changed to be called once for ALL grpc upload types.

@BrennaEpp BrennaEpp requested review from a team January 21, 2025 01:02
@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Jan 21, 2025
@BrennaEpp BrennaEpp mentioned this pull request Jan 21, 2025
Merged
tritone
tritone approved these changes Jan 21, 2025
View reviewed changes

@tritone tritone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjc25 I think this looks like a good change but just FYI.

@cjc25

cjc25 commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

@cjc25 I think this looks like a good change but just FYI.

LGTM. The stashed context was mirroring the implementation as of Nov 2024, which also stashed a context (and also didn't use run):

google-cloud-go/storage/grpc_client.go

Line 2030 in 19f01c3

ctx context.Context
. I think @BrennaEpp is right that the prior behavior didn't handle retry/backoff correctly. I did not merge #11200 correctly, which is how we lost it.

@tritone

tritone commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

@cjc25 I think this looks like a good change but just FYI.

LGTM. The stashed context was mirroring the implementation as of Nov 2024, which also stashed a context (and also didn't use run):

google-cloud-go/storage/grpc_client.go

Line 2030 in 19f01c3

ctx context.Context

. I think @BrennaEpp is right that the prior behavior didn't handle retry/backoff correctly. I did not merge #11200 correctly, which is how we lost it.

Re: #11200 , that was expected, @BrennaEpp re-implemented the retry/backoff behavior in the private repo given the large changes in uploads overall.

@BrennaEpp

BrennaEpp commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

@cjc25 I think this looks like a good change but just FYI.

LGTM. The stashed context was mirroring the implementation as of Nov 2024, which also stashed a context (and also didn't use run):

google-cloud-go/storage/grpc_client.go

Line 2030 in 19f01c3

ctx context.Context

. I think @BrennaEpp is right that the prior behavior didn't handle retry/backoff correctly. I did not merge #11200 correctly, which is how we lost it.

Re: #11200 , that was expected, @BrennaEpp re-implemented the retry/backoff behavior in the private repo given the large changes in uploads overall.

+1, when I introduced run to get the backoff I didn't think about the ctx from run being lost; really, it has never used that ctx and this change adds it in.

@BrennaEpp BrennaEpp enabled auto-merge (squash) January 27, 2025 17:34
@BrennaEpp BrennaEpp merged commit 9ad9d76 into googleapis:main Jan 27, 2025
@BrennaEpp BrennaEpp deleted the fix-ctxpropagation branch January 27, 2025 18:52
@release-please release-please Bot mentioned this pull request Jan 24, 2025
Merged
This was referenced Mar 12, 2025
Closed
Merged
@Bobi-Wan Bobi-Wan mentioned this pull request Apr 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tritone tritone tritone approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BrennaEpp @cjc25 @tritone