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.

Sign up for GitHub

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

Fix 'Client.Endpoint' to not 'cancel' when bufferedStream #776

Merged
merged 4 commits into from
Oct 22, 2018
Merged

Fix 'Client.Endpoint' to not 'cancel' when bufferedStream #776

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8e2326
transport/http/client_test: Modify the test to make it fail
xescugc Oct 18, 2018
1fd618d
transport/http/client: Add the 'bodyWithCancel' to wrap the Response.…
xescugc Oct 18, 2018
d5a7f52
transport/http/client: Add more documentation to clarify the changes
xescugc Oct 20, 2018
f37c50f
transport/http/client: Add period at the end of the doc of 'BufferedS…
xescugc Oct 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions transport/http/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"encoding/json"
"encoding/xml"
"io"
"io/ioutil"
"net/http"
"net/url"
Expand Down Expand Up @@ -92,7 +93,6 @@ func BufferedStream(buffered bool) ClientOption {
func (c Client) Endpoint() endpoint.Endpoint {
return func(ctx context.Context, request interface{}) (interface{}, error) {
ctx, cancel := context.WithCancel(ctx)
defer cancel()

var (
resp *http.Response
Expand All @@ -112,10 +112,12 @@ func (c Client) Endpoint() endpoint.Endpoint {

req, err := http.NewRequest(c.method, c.tgt.String(), nil)
if err != nil {
cancel()
return nil, err
}

if err = c.enc(ctx, req, request); err != nil {
cancel()
return nil, err
}

Expand All @@ -126,11 +128,15 @@ func (c Client) Endpoint() endpoint.Endpoint {
resp, err = c.client.Do(req.WithContext(ctx))

if err != nil {
cancel()
return nil, err
}

if !c.bufferedStream {
if c.bufferedStream {
resp.Body = bodyWithCancel{ReadCloser: resp.Body, cancel: cancel}
Copy link
Member

Choose a reason for hiding this comment

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

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.

} else {
defer resp.Body.Close()
defer cancel()
}

for _, f := range c.after {
Expand All @@ -146,6 +152,20 @@ func (c Client) Endpoint() endpoint.Endpoint {
}
}

// bodyWithCancel is a wrapper for an io.ReadCloser with also a
// cancel function which is called when the Close is used
type bodyWithCancel struct {
io.ReadCloser

cancel context.CancelFunc
}

func (bwc bodyWithCancel) Close() error {
bwc.ReadCloser.Close()
bwc.cancel()
return nil
}

// ClientFinalizerFunc can be used to perform work at the end of a client HTTP
// request, after the response is returned. The principal
// intended use is for error logging. Additional response parameters are
Expand Down
4 changes: 3 additions & 1 deletion transport/http/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func TestHTTPClient(t *testing.T) {

func TestHTTPClientBufferedStream(t *testing.T) {
var (
testbody = "testbody"
testbody = string(make([]byte, 6000))
Copy link
Member

Choose a reason for hiding this comment

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

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)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍

encode = func(context.Context, *http.Request, interface{}) error { return nil }
decode = func(_ context.Context, r *http.Response) (interface{}, error) {
return TestResponse{r.Body, ""}, nil
Expand Down Expand Up @@ -129,6 +129,8 @@ func TestHTTPClientBufferedStream(t *testing.T) {
if !ok {
t.Fatal("response should be TestResponse")
}
defer response.Body.Close()
time.Sleep(time.Second * 1)

// Check that response body was NOT closed
b := make([]byte, len(testbody))
Expand Down