Skip to content
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

Add request context to handler contexts #361

Closed
wants to merge 3 commits into from

Conversation

andrewmostello
Copy link

This pull request changes the http handlers to use the context provided in the request. This change is proposed so that the handlers are able to receive values set in the context in any upstream middleware.

I ran into this issue because we set an authorization value in the context, which I'm using downstream in a custom filestore. This doesn't work if the background context is used in the handlers, as the chain of context passed from above is broken.

Thanks!

@Acconut
Copy link
Member

Acconut commented Mar 16, 2020

Thank you very much for this PR but this patch would break a few things. The reasons for that were laid out in another PR which did a similar thing: #309 (comment)

@andrewmostello
Copy link
Author

Ah ok. Would that issue be handled by this? https://github.com/tus/tusd/blob/master/pkg/handler/unrouted_handler.go#L619

I figured that context.Background() was explicitly used there for a long lived issue like that.

@Acconut
Copy link
Member

Acconut commented Mar 22, 2020

Ah ok. Would that issue be handled by this?

What exactly do you mean by that? I don't understand the question.

The problem is that some stores (e.g. the S3Store) must perform network operation even after the HTTP request is canceled in order to store the transmitted data. So we cannot use the HTTP request's context there or otherwise these network operation would be immediately canceled as well.

@andrewmostello
Copy link
Author

Apologies - I misunderstood. Would this now be an acceptable solution instead? Instead of using the request context, use context.Background, but store the request context as a context value. That can be retrieved later using the RequestContext function, and you can then get at the request context values.

Context used must be active longer than request context. To make
request context available for extracting values, set the request
context as a value of the new context used. Request context may be
retrieved using the RequestContext function.
@andrewmostello andrewmostello changed the title Use request context in handlers Add request context handler context Mar 22, 2020
@andrewmostello andrewmostello changed the title Add request context handler context Add request context to handler contexts Mar 22, 2020
@Acconut
Copy link
Member

Acconut commented May 1, 2020

@andrewmostello Deep apologies for my delay. I somehow lost track of this PR!

Would this now be an acceptable solution instead? Instead of using the request context, use context.Background, but store the request context as a context value.

Frankly, I think it would be a better approach to copy the context values into a new context which does not have a deadline. This way, the design is more in sync with the typical usage of contexts in Go. I explained this in #315 (comment) and @innovate-invent started implementing it in #342. What do you think?

@andrewmostello
Copy link
Author

@Acconut No problem - I've been using my fork for the time being.

That works for me, and I agree that it's cleaner. I'll close out this PR in favor of #342. Thanks!

Frankly, I think it would be a better approach to copy the context values into a new context which does not have a deadline. This way, the design is more in sync with the typical usage of contexts in Go. I explained this in #315 (comment) and @innovate-invent started implementing it in #342. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants