Empty api.Response.Body returned when Content-Length is non-zero

[ebilling]

Describe the bug
In an integration test, I started noticing about a month ago that repeated requests would occasionally return an error. After much digging, I found that the api.Response.Body contained no data, while the Content-Length was correctly set.

After much more digging, I discovered the problem. In function:

func (c *Client) RawRequest(r *Request) (*Response, error) {

.........

var result *Response
resp, err := client.Do(req)
if cancelFunc != nil {
	cancelFunc()
}
if resp != nil {
	result = &Response{Response: resp}
}

should be:

if cancelFunc != nil {
	defer cancelFunc()
}
var result *Response
resp, err := client.Do(req)
if resp != nil {
	result = &Response{Response: resp}
}

see documentation for WithTimeout:

// WithTimeout returns WithDeadline(parent, time.Now().Add(timeout)).
//
// Canceling this context releases resources associated with it, so code should
// call cancel as soon as the operations running in this Context complete:
//
// func slowOperationWithTimeout(ctx context.Context) (Result, error) {
// ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond)
// defer cancel() // releases resources if slowOperation completes before timeout elapses
// return slowOperation(ctx)
// }

To Reproduce
Steps to reproduce the behavior:
1.) Using the go API, set up a loop that reads the same secret repeatedly with a low timeout (I used 1 second) on the same system that's running the instance of vault you are testing against.
2.) Track whether the secret is returned.

Expected behavior
If the data has been returned to the client, it should be returned.

Environment:

• Vault Server Version (retrieve with vault status): Version 0.10.3
• Vault CLI Version (retrieve with vault version): Vault v0.10.3 (cgo)
• Server Operating System/Architecture: MacOS and Linux

[jefferai]

The problem with deferring is that we support retries so these could pile up. However, it's good to know that canceling a context will actually wipe the data. The documentation is frustratingly vague for what "releases sesources associated with it means" since that's going to depend strongly on individual package behavior.

[jefferai]

Can you check out the cancelshift branch and see if that fixes things for you?

[ebilling]

I’ll try it, but I don’t think it will work. The cancelFunc call needs to be deferred. Otherwise, you have race conditions for when it is actually called, leading to unpredictable behavior, especially under load.

…

On Jul 23, 2018, at 8:17 AM, Jeff Mitchell ***@***.***> wrote: Can you check out the cancelshift branch and see if that fixes things for you? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4973 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADm0j_tzQWRaecAE8yLbz5trvsOhqq-eks5uJejzgaJpZM4VaUlG>.

[briankassouf]

@ebilling would you mind testing #4987

[briankassouf]

@ebilling in case you are looking at email i meant #4987

[jefferai]

(Also see #4987 (comment))