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.

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

net/http: client doesn't check for canceled context (delegates it to the Transport) #46206

Open
ehsangifani opened this issue May 17, 2021 · 5 comments

Comments

@ehsangifani
Copy link

@ehsangifani ehsangifani commented May 17, 2021

What version of Go are you using (go version)?

go version go1.15.6 linux/amd64

Does this issue reproduce with the latest release?

Yes. Also on go1.16.4

What operating system and processor architecture are you using (go env)?

go env Output
linux amd64

What did you do?

package golanghttpbug

import (
	"bytes"
	"context"
	"io/ioutil"
	"net/http"
	"testing"
)

type roundTripperFunc func(req *http.Request) (*http.Response, error)

func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) {
	return f(r)
}

func createClient(checkCtx bool) *http.Client {
	return &http.Client{
		Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) {
			resp := &http.Response{
				Body:   ioutil.NopCloser(bytes.NewReader([]byte("a response"))),
				Header: http.Header{},
			}
			if checkCtx && req.Context().Err() != nil {
				return resp, http.ErrHandlerTimeout
			}
			return resp, nil
		}),
	}
}

func TestRequest(t *testing.T) {
	canceledCtx, cancel := context.WithCancel(context.Background())
	cancel()
	tests := []struct {
		name    string
		client  *http.Client
		wantErr bool
	}{
		{
			name:   "default transport handles canceld context",
			client: http.DefaultClient,
		},
		{
			name:    "http package itself doesn't check for canceled context",
			client:  createClient(false),
			wantErr: true,
		},
		{
			name:   "undocumented: custom transport might receive an already canceled context",
			client: createClient(true),
		},
	}

	for _, tt := range tests {
		tt := tt
		t.Run(tt.name, func(t *testing.T) {
			req, err := http.NewRequestWithContext(canceledCtx, http.MethodGet, "https://www.google.com/", http.NoBody)
			if err != nil {
				t.Fatalf("unexpected error: %v", err)
			}
			resp, err := tt.client.Do(req)
			if err == nil {
				t.Errorf("request not canceled: %v", err)
			}
			if resp != nil {
				defer resp.Body.Close()
				raw, err := ioutil.ReadAll(resp.Body)
				if err == nil {
					t.Logf("response len: %v\n", len(raw))
					t.Fatalf("request body readable: %v", err)
				}
			}
		})
	}
}

What did you expect to see?

Wanted all tests to pass

What did you see instead?

undocumented behavior.

There is commit that has added a test for another issue. But as it spawns a server, it doesn't see the problem when using custom transport.

The request context is checked here but it only checks for deadline and not cancelations.

Also as must people use the DefaultTransport this bug does not show itself. But it is an undocumented behavior which either needs to be fixed(which is an easy task) or it must be documented that a RoundTripper must handle this.

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 17, 2021

@dmitshur dmitshur added this to the Backlog milestone May 17, 2021
@neild
Copy link
Contributor

@neild neild commented May 17, 2021

A RoundTripper should check for the request context becoming done, either from a timeout or from an explicit cancelation.

Even if Client.Do checked for an already-canceled context, a request context could still be canceled after the call to RoundTrip. In this case, it seems obvious that RoundTrip needs to handle the cancelation. Since RoundTrip needs to handle cancelation in this case, any check in Client.Do will be redundant.

Perhaps there could be additional documentation on RoundTrip explicitly stating that it should honor the request context becoming done, but it seems to me to be implicit in the existence of request cancelation. If the RoundTripper doesn't honor the context being canceled, then canceling the context will obviously not cause RoundTrip to return.

@ehsangifani
Copy link
Author

@ehsangifani ehsangifani commented May 18, 2021

@neild
I agree with your argument. However, if you check the second link, context is being checked for deadline already. So we can also argue that this check is redundant as well; unless timeouts and cancellations are logically different from the net/http point of view.

@neild
Copy link
Contributor

@neild neild commented May 18, 2021

However, if you check the second link, context is being checked for deadline already.

The only uses of that function that I see are to determine whether to create a new context.WithDeadline when Client.Timeout is set: If the request has a context that expires before the timeout, then don't bother setting a new deadline.

Nothing that I can see checks to see if the deadline has already expired.

@ehsangifani
Copy link
Author

@ehsangifani ehsangifani commented May 19, 2021

The only uses of that function that I see are to determine whether to create a new context.WithDeadline when Client.Timeout is set

Oh right. I misunderstood. Then I think a documentation fix would do the trick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants