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

Request context is lost #457

Closed
ferdypruis opened this issue Nov 30, 2020 · 4 comments
Closed

Request context is lost #457

ferdypruis opened this issue Nov 30, 2020 · 4 comments

Comments

@ferdypruis
Copy link

ferdypruis commented Nov 30, 2020

In oauth2.Transport.RoundTrip the given request is cloned so it may be changed (auth header added), as dictated by the Roundtripper-contract. But this clone does not include the request context.

This can easily be fixed.

Index: transport.go
IDEA additional info:
<+>UTF-8
===================================================================
--- transport.go	(revision 9fd604954f58d391cab0f7f49affb0a6aa864086)
+++ transport.go	(date 1606751247213)
@@ -80,6 +80,7 @@
 	// shallow copy of the struct
 	r2 := new(http.Request)
 	*r2 = *r
+	r2 = r2.WithContext(r.Context())
 	// deep copy of the Header
 	r2.Header = make(http.Header, len(r.Header))
 	for k, s := range r.Header {

Current versions of Go have a http.Request.Clone receiver which clones even more fields, which might serve as inspiration.

ferdypruis pushed a commit to ferdypruis/oauth2 that referenced this issue Nov 30, 2020
ferdypruis pushed a commit to ferdypruis/oauth2 that referenced this issue Oct 1, 2021
jwijenbergh added a commit to jwijenbergh/oauth2 that referenced this issue Apr 28, 2023
@willmo
Copy link

willmo commented Jul 12, 2023

Doesn't *r2 = *r copy the field .ctx? (Sorry if I'm missing something.)

There are a lot of other context-related bugs and confusions though (#262 #324 #388 #564...).

@ferdypruis
Copy link
Author

ferdypruis commented Jul 12, 2023

Doesn't *r2 = *r copy the field .ctx? (Sorry if I'm missing something.)

Because the ctx field is unexported it is not accessible and therefor not copied.

@willmo
Copy link

willmo commented Jul 14, 2023

It works for me: https://go.dev/play/p/Sdjef2JUQ1S . And see also https://go.dev/doc/go1#unexported .

I think using .Clone instead makes sense though.

@ferdypruis
Copy link
Author

ferdypruis commented Jul 14, 2023

You have me confused here. I was convinced I had the issue of my context not being copied and fixed it with this change.
But you are right.

Your test works and the Go specification clearly states;

Behind the scenes the unexported fields will be assigned and copied just as if they were exported, but the client code will never be aware of them.

I can not reproduce the issue I thought I had, neither when downgrading back to Go1.11. The pointer to the context is always copied.

There might be other issues with this shallow clone that using the new .Clone() will fix, but that's not what this issue was about and I will therefor close.
Thank you for pointing this out @willmo.

jwijenbergh added a commit to jwijenbergh/oauth2 that referenced this issue Dec 14, 2023
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

No branches or pull requests

2 participants