Fix 'Client.Endpoint' to not 'cancel' when bufferedStream #776
Conversation
With this modifications we can trigger the error that we are searching, 'context canceled'
…Body It adds the context.CancelFunc to the io.ReadCloser.Close function so bouth are called together
transport/http/client_test.go
Outdated
| @@ -99,7 +99,7 @@ func TestHTTPClient(t *testing.T) { | |||
|
|
|||
| func TestHTTPClientBufferedStream(t *testing.T) { | |||
| var ( | |||
| testbody = "testbody" | |||
| testbody = string(make([]byte, 6000)) | |||
There was a problem hiding this comment.
Perhaps a comment explaining the choice of 6000? Or, better yet, factor it out into a const, e.g.
var (
bodysize = 6000 // trigger the foo in the bar package
testbody = bytes.Repeat('a', bodysize)
)There was a problem hiding this comment.
I has not an specific meaning by itself, it big enough to make the test fail hehe. If it where to be over 9000 then we would have to modify the test as the _, err = response.Body.Read(b) would not return io.EOF but on the second read would do it (I explained this on the main issue hehe).
I added the documentation too 👍
| return nil, err | ||
| } | ||
|
|
||
| if !c.bufferedStream { | ||
| if c.bufferedStream { | ||
| resp.Body = bodyWithCancel{ReadCloser: resp.Body, cancel: cancel} |
There was a problem hiding this comment.
A comment here to motivate this wrapper would also be welcome. Something like this, feel free to rephrase if I got something wrong:
// If we expect a buffered stream, we don't cancel the context when the endpoint returns.
// Instead, we should call the cancel func when closing the response body.Also abstracted some logic on the test to make it more clear and also added more docuemntation. Added more documentation on the definition of 'BufferedStream' to clarify that the Body has to be closed manually to properly close the response.
|
I've also added documentation on the About the |
| // bodysize has a size big enought to make the resopnse.Body not an instant read | ||
| // so if the response is cancelled it wount be all readed and the test would fail | ||
| // The 6000 has not a particular meaning, it big enough to fulfill the usecase. | ||
| const bodysize = 6000 |
There was a problem hiding this comment.
I guess my question was more like: why is 6000 big enough, and 9000 too big? Is there a constant or default buffer size or something defined in the stdlib net/http package? If so, can you refer to that?
There was a problem hiding this comment.
I would also want to know that ... but I've tried to search for it, where the body is setted on a response and how is the io.ReadCoser is implemented on it, but It was impossible to find it for me, I would really like to have that answer because then I could provide a more accurate answer on what it's happening.
Also the 9000 it's not "too big" but it simply makes the io.Read works differently then. From the io.Read
[...] An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF. [...]
So the next read to the body would return the io.EOF instead of the first one (I did try it while testing, but It's not something realted to go-kit but to the stdlib so IMO no test needed for this).
I think is on the way the body is read, I think that it's read by "chunks" from the "sender" instead of buffering all the body.
transport/http/client.go
Outdated
| @@ -84,6 +85,7 @@ func ClientFinalizer(f ...ClientFinalizerFunc) ClientOption { | |||
|
|
|||
| // BufferedStream sets whether the Response.Body is left open, allowing it | |||
| // to be read from later. Useful for transporting a file as a buffered stream. | |||
| // That body has to be Closed to propery end the request | |||
|
@xescugc Nice solution with the |
Fixes #773 by adding a wrapper to the
Resonse.Bodywhen thebufferedStreamis specified.For now it's a PoC as it may change.
The biggest drawback is that old users of
bufferedStreamthat where not closing theResopnse.Bodymay end up with an open connection 😢 . The only solution I can have for that is to have another option to tell the client that the user will close the connection, so all this logic I did would be placed in an if condition of that other option (which maybe cleaner, but with the bug onbufferedStreamwill still be there).To make the test fail I had to fake a "big" body (
string(make([]byte, 6000))) and "fake work" (time.Sleep), then those where fixed with the implementation.Depending on how does this evolve, I could include the jsonrpc.Client.Endpoint to this PR or a new PR, as it has the same/similar implementation so I suppose the same "bug" (did not try it).