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

Modify c.Request.Context() when using ContextWithFallback #3367

Closed
wants to merge 1 commit into from

Conversation

bvisness
Copy link

@bvisness bvisness commented Oct 18, 2022

Gin 1.8 added the ContextWithFallback feature flag, which causes the context.Context methods to fall back to c.Request.Context(). Presumably, the intent of this feature was to cause the Gin context to be a part of the overall context chain of the request. However, this intent breaks down when using gin.WrapH with a third-party http.Handler.

Because the signature of an http.Handler does not include a *gin.Context, an http.Handler can only access req.Context(). However, this always refers to the request's original context instead of the Gin context. This means that any values set with Gin's c.Set are not accessible in those handlers. This means that Gin middleware that sets context values actually can't pass values to third-party handlers!

This PR therefore sets c.Request.Context() to be the Gin context itself. That way, req.Context() in a third-party handler returns the Gin context, and can get the values it expects.

It only does this when using ContextWithFallback, to avoid breakage. I figure this is probably enough breakage-avoidance, because this flag is already opt-in and most people won't opt in. However, if you needed to avoid further breakage, I could perhaps update this to only do this work when using gin.WrapH.

Refs #2751, #2769, #3166, #3172

@bvisness
Copy link
Author

Naturally, minutes after finishing this, I thought of a much less invasive fix for our problem. I have opened #3368 as a result. Closing this.

@bvisness bvisness closed this Oct 18, 2022
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.

1 participant