Go-github: UnauthenticatedRateLimitedTransport.RoundTrip breaks http.RoundTripper contract (mostly harmless).

[dmitshur]

UnauthenticatedRateLimitedTransport.RoundTrip does this:

// To set extra querystring params, we must make a copy of the Request so
// that we don't modify the Request we were given. This is required by the
// specification of http.RoundTripper.
req = cloneRequest(req)
q := req.URL.Query()
q.Set("client_id", t.ClientID)
q.Set("client_secret", t.ClientSecret)
req.URL.RawQuery = q.Encode()

But cloneRequest doesn't make a copy of the URL field, it still points to the same url.URL struct that the original request points to. So doing req.URL.RawQuery = q.Encode() actually modifies the URL of original request.

This issue is mostly harmless, but it's a bug.

This is similar to https://go-review.googlesource.com/c/36483/.

[karanjthakkar]

@shurcooL I was interested in picking this up. Just wanted to clarify if updating the cloneRequest implementation to make a deep copy of the req.URL is the expected solution or something else?

[dmitshur]

@karanjthakkar Sounds great, thanks for considering looking into this!

Actually, figuring out the best approach to go with is a part of resolving this issue. The one you said is one valid approach, but I can think of a few others, with trade-offs (such as less code vs better performance) to be considered.

cloneRequest is used in exactly 2 places, in UnauthenticatedRateLimitedTransport.RoundTrip and inside BasicAuthTransport.RoundTrip. However, each has slightly different needs at this time:

• UnauthenticatedRateLimitedTransport.RoundTrip needs only to copy the request and its URL field, since it only modifies URL and doesn't touch headers.
• BasicAuthTransport.RoundTrip, on the other hand, needs only to copy the request and the Headers map, because it only modifies Headers and not the URL.

It's possible to change cloneRequest to make copies of both URL and Headers, then cloneRequest is usable for both uses.

However, that might be a little wasteful, and maybe it'd be better to create 2 separate cloneRequest funcs for the 2 separate needs.

Or maybe it's better to just inline the code, customize it for the needs of each caller, and remove cloneRequest altogether.

Factors to consider when making the decision is performance (is this significant enough of a difference to matter?), GC pressure, and readability/maintainability of the code, and likelihood of how things may change in the future.

Do you want to look into that and make a suggestion for what you think would be the best path to take?

[karanjthakkar]

@shurcooL Thanks for the detailed explanation!

As for readability, I think that a separate method for cloning makes sense. However for maintainability, I don't believe having two separate methods would help. It would really depend on how we can scale this method in the future if more request params would need to be deep copy'ed.

Also, honestly, I have literally just started learning Go so, while I can research a bit on how GC would differ in case we deep copied everything, it would take me a little more time and effort than I thought I would need to make this work. Would you mind pointing me to resources that would help me investigate GC for this case?

As for likelihood of how things may change in the future, apart from the fact that we may need to deep copy more params, I'm not sure how to gauge that. Sorry!

[dmitshur]

A way to answer the performance and allocation amount questions analytically is to write a benchmark that stress tests the RoundTrip method, and implement both alternatives, then see how they compare in performance and allocations made.

Given how short and simple both UnauthenticatedRateLimitedTransport.RoundTrip and BasicAuthTransport.RoundTrip are, I think the simplest and best solution would be to just inline the functionality of cloneRequest into each, adjust it for the needs of each, and then remove the unused cloneRequest func. That's what I would suggest doing.

I would spend time on profiling if cloneRequest was used in more than 2 places and it was very desirable to keep it. It's just not worth it for the current situation.

[sahildua2305]

@karanjthakkar are you still interested in working on this?

[karanjthakkar]

@sahildua2305 Sorry for the inactivity. I don't think I'll be able to work on this anytime soon. :(

If someone else is interested feel free to pick this up.

[sahildua2305]

@shurcooL I'd like to take this one down next. Can you please help me understand what needs to be done here?

[dmitshur]

I'd like to take this one down next.

@sahildua2305 Great, thanks!

Can you please help me understand what needs to be done here?

Start by reading the original issue description in full, it should have all the neccessary information about the issue (let me know if not).

As for the way to organize the code, see the suggestion I made in my last comment above:

Given how short and simple both UnauthenticatedRateLimitedTransport.RoundTrip and BasicAuthTransport.RoundTrip are, I think the simplest and best solution would be to just inline the functionality of cloneRequest into each, adjust it for the needs of each, and then remove the unused cloneRequest func. That's what I would suggest doing.

[psdh]

@sahildua2305, Are you planning to pick this up?

(side note: I ended up at go-github after reading your post about learning Go while contributing to FOSS, thanks! :) )

[sahildua2305]

Hi @psdh, yes, I started working on it a while ago but then got caught up with something else. Sorry for not updating the issue for a long time. I'll submit a PR soon.

I ended up at go-github after reading your post about learning Go while contributing to FOSS, thanks! :)

Wow! That's amazing. 😍
Please hang around and have a look at other open issue and feel free to ask for help.