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

contrib/net/http: copy request in RoundTrip #1254

Merged
merged 13 commits into from
Jul 7, 2022

Conversation

mackjmr
Copy link
Member

@mackjmr mackjmr commented Apr 22, 2022

This PR copy's the Request in the RoundTrip() function to comply with the RoundTripper spec (link):

// RoundTrip should not modify the request, except for
// consuming and closing the Request's Body. RoundTrip may
// read fields of the request in a separate goroutine. Callers
// should not mutate or reuse the request until the Response's
// Body has been closed.

With this change, a shallow copy of the request is made (r.WithContext(ctx) returns a shallow copy of r), and then a deep copy of the Header map is made before calling tracer.Inject().

Fixes: #1090

@mackjmr mackjmr requested a review from a team April 22, 2022 12:06
@ajgajg1134 ajgajg1134 added this to the 1.39.0 milestone Apr 22, 2022
ajgajg1134
ajgajg1134 previously approved these changes Apr 22, 2022
Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks!

knusbaum
knusbaum previously approved these changes May 3, 2022
@knusbaum
Copy link
Contributor

knusbaum commented May 3, 2022

It would be nice to be able to use (*Request).Clone (https://pkg.go.dev/net/http#Request.Clone) available in go1.13.

We're in the process of updating our supported Go versions which will push 1.12 out of "supported" status.

@Hellzy Hellzy modified the milestones: 1.39.0, 1.40.0 Jul 1, 2022
@mackjmr mackjmr dismissed stale reviews from knusbaum and ajgajg1134 via e84f94a July 6, 2022 13:29
@ajgajg1134 ajgajg1134 merged commit e5fb074 into main Jul 7, 2022
@ajgajg1134 ajgajg1134 deleted the mackjmr/roundtrip-duplicate-request branch July 7, 2022 19:11
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.

contrib/net/http: Roundtripper should not modify request
6 participants