Skip to content

Commit

Permalink
lib/netext/httpext: set request context explicitly
Browse files Browse the repository at this point in the history
Go 1.13 and below has a bug that client timeout will not be propagated
to http request context. That bug was fixed already and will be present
in go1.14 (golang/go#31657).

Currently, we set Timeout in our Client, but do not use it else where.
It will not affect the request context for now, but does when go1.14
release.

To make it compatible and works correctly with other go versions, we set
the request context explicitly with the timeout.

Update #1260
  • Loading branch information
cuonglm committed Dec 17, 2019
1 parent c7b5c7f commit ef6d3ce
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 4 deletions.
7 changes: 4 additions & 3 deletions lib/netext/httpext/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,6 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
resp := &Response{ctx: ctx, URL: preq.URL.URL, Request: *respReq}
client := http.Client{
Transport: transport,
Timeout: preq.Timeout,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
resp.URL = req.URL.String()

Expand Down Expand Up @@ -312,7 +311,9 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
},
}

mreq := preq.Req.WithContext(ctx)
reqCtx, cancelFunc := context.WithTimeout(ctx, preq.Timeout)
defer cancelFunc()
mreq := preq.Req.WithContext(reqCtx)
res, resErr := client.Do(mreq)

// TODO(imiric): It would be safer to check for a writeable
Expand Down Expand Up @@ -367,7 +368,7 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
}

if resErr != nil {
// Do *not* log errors about the contex being cancelled.
// Do *not* log errors about the context being cancelled.
select {
case <-ctx.Done():
default:
Expand Down
10 changes: 9 additions & 1 deletion lib/netext/httpext/request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,21 @@ func TestMakeRequestError(t *testing.T) {
defer srv.Close()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
logger := logrus.New()
logger.Level = logrus.DebugLevel
state := &lib.State{
Options: lib.Options{RunTags: &stats.SampleTags{}},
Transport: srv.Client().Transport,
Logger: logger,
}
ctx = lib.WithState(ctx, state)
req, _ := http.NewRequest("GET", srv.URL, nil)
var preq = &ParsedHTTPRequest{Req: req, URL: &URL{u: req.URL}, Body: new(bytes.Buffer)}
var preq = &ParsedHTTPRequest{
Req: req,
URL: &URL{u: req.URL},
Body: new(bytes.Buffer),
Timeout: 10 * time.Second,
}

res, err := MakeRequest(ctx, preq)

Expand Down

0 comments on commit ef6d3ce

Please sign in to comment.