Skip to content

Commit

Permalink
internal: don't cancel http contexts until the body has been read
Browse files Browse the repository at this point in the history
Contexts for HTTP requests should not be cancelled until after the HTTP
body has been successfully read, or errors can be encountered.
  • Loading branch information
Derek Gonyeo committed May 23, 2018
1 parent 8bc8a13 commit c7d843a
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 11 deletions.
19 changes: 10 additions & 9 deletions internal/resource/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,13 +194,14 @@ func (f *Fetcher) newHttpClient() {
}
}

// getReaderWithHeader performs an HTTP GET on the provided URL with the provided request header
// and returns the response body Reader, HTTP status code, and error (if any). By
// getReaderWithHeader performs an HTTP GET on the provided URL with the
// provided request header and returns the response body Reader, HTTP status
// code, a cancel function for the result's context, and error (if any). By
// default, User-Agent is added to the header but this can be overridden.
func (c HttpClient) getReaderWithHeader(ctx context.Context, url string, header http.Header) (io.ReadCloser, int, error) {
func (c HttpClient) getReaderWithHeader(url string, header http.Header) (io.ReadCloser, int, context.CancelFunc, error) {
req, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil, 0, err
return nil, 0, nil, err
}

req.Header.Set("User-Agent", "Ignition/"+version.Raw)
Expand All @@ -212,10 +213,10 @@ func (c HttpClient) getReaderWithHeader(ctx context.Context, url string, header
}
}

ctx, cancelFn := context.WithCancel(context.Background())
if c.timeout != 0 {
ctxTo, cancel := context.WithTimeout(ctx, c.timeout)
ctx = ctxTo
defer cancel()
cancelFn()
ctx, cancelFn = context.WithTimeout(context.Background(), c.timeout)
}

duration := initialBackoff
Expand All @@ -226,7 +227,7 @@ func (c HttpClient) getReaderWithHeader(ctx context.Context, url string, header
if err == nil {
c.logger.Info("GET result: %s", http.StatusText(resp.StatusCode))
if resp.StatusCode < 500 {
return resp.Body, resp.StatusCode, nil
return resp.Body, resp.StatusCode, cancelFn, nil
}
resp.Body.Close()
} else {
Expand All @@ -242,7 +243,7 @@ func (c HttpClient) getReaderWithHeader(ctx context.Context, url string, header
select {
case <-time.After(duration):
case <-ctx.Done():
return nil, 0, ErrTimeout
return nil, 0, cancelFn, ErrTimeout
}
}
}
8 changes: 6 additions & 2 deletions internal/resource/url.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,11 +218,15 @@ func (f *Fetcher) FetchFromTFTP(u url.URL, dest *os.File, opts FetchOptions) err
// FetchFromHTTP fetches a resource from u via HTTP(S) into dest, returning an
// error if one is encountered.
func (f *Fetcher) FetchFromHTTP(u url.URL, dest *os.File, opts FetchOptions) error {
ctx := context.Background()
if f.client == nil {
f.newHttpClient()
}
dataReader, status, err := f.client.getReaderWithHeader(ctx, u.String(), opts.Headers)
dataReader, status, ctxCancel, err := f.client.getReaderWithHeader(u.String(), opts.Headers)
if ctxCancel != nil {
// whatever context getReaderWithHeader created for the request should
// be cancelled once we're done reading the response
defer ctxCancel()
}
if err != nil {
return err
}
Expand Down

0 comments on commit c7d843a

Please sign in to comment.