Take shallow copy of HTTP request when copying

[ryanfowler]

Taking a shallow copy of the HTTP request copies over all fields, making this library compatible with all version of Go v1.x (no need for build tags).

Also, as discussed in #75 , this would copy over the HTTP request's context field in Go 1.7+.

[jasdel]

Thanks for creating this Pr @ryanfowler. I agree the SDK should be maintaining the context between requests retries.

[jasdel]

The idea of the changes are good, but regrettably because of the potential race condition doing a shallow copy with Transport.

to follow this pattern I think a +1.7 version of the http_request.go files needs to be created. We could do this by renaming http_request.go to http_request_1_5.go and creating a http_request_1_7.go. If there is a better way to group or share logic between these Go version specific helpers we'd be open to that.

[jasdel]

This was originally done as an explicit copy because of race conditions that could occur between the original request's Body and the underlying client's Transport could still have an outstanding call to r.Body.Read. The Go race checker flagged this as a race condition. This is why the individual fields were copied.

This is also why the Request wraps the original body reader in a offsetReader. So that the underlying buffer can be rewound for the next retry request attempt.

[jasdel]

Because of the above comment, regrettably we still need to keep this version around.

[ryanfowler]

@jasdel thanks for reviewing!

With regards to the race condition inBody, it appears that the only way this would introduce a race condition is if the Body field in the initial request was being assigned to at the same time as the copy operation. I'm not totally sure where this is happening other than in this function.

That's too bad, I was hoping to avoid creating another version specific file for this. However if you'd like to proceed this way, I can update the current http_request.go to http_request_1_5.go with build tags: // +build go1.5,!go1.7, and create a new file that copies the request's context? In that case, all of these commits should be squashed, if accepted.

[jasdel]

Thanks for the update @ryanfowler I'm taking a second look at the SDK and if a shallow copy would be sufficient. I think you're correct that the only place body is assigned other than during retry is the debug logger.

[jasdel]

I'm not able to trigger a race condition with the SDK and this change. I think we are safe to take this change. In more investigation it looks like the conflict between the Body and Transport is resolved because of the usage of the offsetReader, that provides protection against the RouteTripper potentially still reading from the Body after the request call has returned, i.e golang/go#12796.