From 46cdbe2a636036ee39bd0e44266359598b118f7d Mon Sep 17 00:00:00 2001 From: na-- Date: Tue, 6 Aug 2019 16:37:50 +0300 Subject: [PATCH] Fix response error handling and improve digest auth & http-debug code (#1102) Previously, there was an uncaught error (sometimes leading to panics) in the automatic response body decompression. This patch handles these decompression errors and even assigns a custom error code for them. As a bonus, k6 now also properly handles errors due to improper redirects, or too many redirects, and tags the resulting metric samples accordingly. By necessity, I had to also move the digest authentication and http-debug handling to their own http.RoundTripper layers. This by no means solves any of the following issues: - https://github.com/loadimpact/k6/issues/800 - https://github.com/loadimpact/k6/issues/986 - https://github.com/loadimpact/k6/issues/1042 - https://github.com/loadimpact/k6/issues/774 ...but it hopefully slightly improves the situation. For the digest authentication, a proper authentication cache, and very likely a different library, still have to be used... And regarding `http-debug`, a discussion about how some or all the remaining issues can be fixed can be found here: https://github.com/loadimpact/k6/pull/1102#discussion_r310902305 --- js/modules/k6/http/request_test.go | 79 +++++++++-- lib/netext/httpext/digest_transport.go | 84 ++++++++++++ lib/netext/httpext/error_codes.go | 34 +++++ lib/netext/httpext/httpdebug_transport.go | 66 +++++++++ lib/netext/httpext/request.go | 159 +++++++++------------- lib/netext/httpext/response.go | 14 -- lib/netext/httpext/transport.go | 11 +- 7 files changed, 329 insertions(+), 118 deletions(-) create mode 100644 lib/netext/httpext/digest_transport.go create mode 100644 lib/netext/httpext/httpdebug_transport.go diff --git a/js/modules/k6/http/request_test.go b/js/modules/k6/http/request_test.go index 6b2e3719799..25af8aa106b 100644 --- a/js/modules/k6/http/request_test.go +++ b/js/modules/k6/http/request_test.go @@ -45,6 +45,7 @@ import ( "github.com/loadimpact/k6/lib/metrics" "github.com/loadimpact/k6/lib/testutils" "github.com/loadimpact/k6/stats" + "github.com/mccutchen/go-httpbin/httpbin" "github.com/oxtoacart/bpool" "github.com/sirupsen/logrus" logtest "github.com/sirupsen/logrus/hooks/test" @@ -1592,20 +1593,16 @@ func TestErrorCodes(t *testing.T) { script: `let res = http.request("GET", "HTTPBIN_URL/redirect-to?url=http://dafsgdhfjg/");`, }, { - name: "Non location redirect", - expectedErrorCode: 0, - expectedErrorMsg: "", - script: `let res = http.request("GET", "HTTPBIN_URL/no-location-redirect");`, - expectedScriptError: sr(`GoError: Get HTTPBIN_URL/no-location-redirect: 302 response missing Location header`), + name: "Non location redirect", + expectedErrorCode: 1000, + expectedErrorMsg: "302 response missing Location header", + script: `let res = http.request("GET", "HTTPBIN_URL/no-location-redirect");`, }, { name: "Bad location redirect", - expectedErrorCode: 0, - expectedErrorMsg: "", + expectedErrorCode: 1000, + expectedErrorMsg: "failed to parse Location header \"h\\t:/\": parse h\t:/: net/url: invalid control character in URL", //nolint: lll script: `let res = http.request("GET", "HTTPBIN_URL/bad-location-redirect");`, - expectedScriptError: sr( - "GoError: Get HTTPBIN_URL/bad-location-redirect: failed to parse Location header" + - " \"h\\t:/\": parse h\t:/: net/url: invalid control character in URL"), }, { name: "Missing protocol", @@ -1641,7 +1638,7 @@ func TestErrorCodes(t *testing.T) { _, err := common.RunString(rt, sr(testCase.script+"\n"+fmt.Sprintf(` if (res.status != %d) { throw new Error("wrong status: "+ res.status);} - if (res.error != '%s') { throw new Error("wrong error: "+ res.error);} + if (res.error != %q) { throw new Error("wrong error: '" + res.error + "'");} if (res.error_code != %d) { throw new Error("wrong error_code: "+ res.error_code);} `, testCase.status, testCase.expectedErrorMsg, testCase.expectedErrorCode))) if testCase.expectedScriptError == "" { @@ -1824,3 +1821,63 @@ func BenchmarkHandlingOfResponseBodies(b *testing.B) { b.Run("binary", testResponseType("binary")) b.Run("none", testResponseType("none")) } + +func TestErrorsWithDecompression(t *testing.T) { + t.Parallel() + tb, state, _, rt, _ := newRuntime(t) + defer tb.Cleanup() + + state.Options.Throw = null.BoolFrom(false) + + tb.Mux.HandleFunc("/broken-archive", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + enc := r.URL.Query()["encoding"][0] + w.Header().Set("Content-Encoding", enc) + _, _ = fmt.Fprintf(w, "Definitely not %s, but it's all cool...", enc) + })) + + _, err := common.RunString(rt, tb.Replacer.Replace(` + function handleResponseEncodingError (encoding) { + let resp = http.get("HTTPBIN_URL/broken-archive?encoding=" + encoding); + if (resp.error_code != 1701) { + throw new Error("Expected error_code 1701 for '" + encoding +"', but got " + resp.error_code); + } + } + + ["gzip", "deflate", "br", "zstd"].forEach(handleResponseEncodingError); + `)) + assert.NoError(t, err) +} + +func TestDigestAuthWithBody(t *testing.T) { + t.Parallel() + tb, state, samples, rt, _ := newRuntime(t) + defer tb.Cleanup() + + state.Options.Throw = null.BoolFrom(true) + state.Options.HttpDebug = null.StringFrom("full") + + tb.Mux.HandleFunc("/digest-auth-with-post/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "POST", r.Method) + body, err := ioutil.ReadAll(r.Body) + require.NoError(t, err) + require.Equal(t, "super secret body", string(body)) + httpbin.New().DigestAuth(w, r) // this doesn't read the body + })) + + // TODO: fix, the metric tags shouldn't have credentials (https://github.com/loadimpact/k6/issues/1103) + urlWithCreds := tb.Replacer.Replace( + "http://testuser:testpwd@HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd", + ) + + _, err := common.RunString(rt, fmt.Sprintf(` + let res = http.post(%q, "super secret body", { auth: "digest" }); + if (res.status !== 200) { throw new Error("wrong status: " + res.status); } + if (res.error_code !== 0) { throw new Error("wrong error code: " + res.error_code); } + `, urlWithCreds)) + require.NoError(t, err) + + expectedURL := tb.Replacer.Replace("HTTPBIN_IP_URL/digest-auth-with-post/auth/testuser/testpwd") + sampleContainers := stats.GetBufferedSamples(samples) + assertRequestMetricsEmitted(t, sampleContainers[0:1], "POST", expectedURL, urlWithCreds, 401, "") + assertRequestMetricsEmitted(t, sampleContainers[1:2], "POST", expectedURL, urlWithCreds, 200, "") +} diff --git a/lib/netext/httpext/digest_transport.go b/lib/netext/httpext/digest_transport.go new file mode 100644 index 00000000000..5eece2379e7 --- /dev/null +++ b/lib/netext/httpext/digest_transport.go @@ -0,0 +1,84 @@ +/* + * + * k6 - a next-generation load testing tool + * Copyright (C) 2019 Load Impact + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +package httpext + +import ( + "io/ioutil" + "net/http" + + digest "github.com/Soontao/goHttpDigestClient" +) + +type digestTransport struct { + originalTransport http.RoundTripper +} + +// RoundTrip handles digest auth by behaving like an http.RoundTripper +// +// TODO: fix - this is a preliminary solution and is somewhat broken! we're +// always making 2 HTTP requests when digest authentication is enabled... we +// should cache the nonces and behave more like a browser... or we should +// ditch the hacky http.RoundTripper approach and write our own client... +// +// Github issue: https://github.com/loadimpact/k6/issues/800 +func (t digestTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Make the initial request authentication params to compute the + // authorization header + username := req.URL.User.Username() + password, _ := req.URL.User.Password() + + // Remove the user data from the URL to avoid sending the authorization + // header for basic auth + req.URL.User = nil + + noAuthResponse, err := t.originalTransport.RoundTrip(req) + if err != nil || noAuthResponse.StatusCode != http.StatusUnauthorized { + // If there was an error, or if the remote server didn't respond with + // status 401, we simply return, so the upstream code can deal with it. + return noAuthResponse, err + } + + respBody, err := ioutil.ReadAll(noAuthResponse.Body) + if err != nil { + return nil, err + } + _ = noAuthResponse.Body.Close() + + // Calculate the Authorization header + // TODO: determine if we actually need the body, since I'm not sure that's + // what the `entity` means... maybe a moot point if we change the used + // digest auth library... + challenge := digest.GetChallengeFromHeader(&noAuthResponse.Header) + challenge.ComputeResponse(req.Method, req.URL.RequestURI(), string(respBody), username, password) + authorization := challenge.ToAuthorizationStr() + req.Header.Set(digest.KEY_AUTHORIZATION, authorization) + + if req.GetBody != nil { + // Reset the request body if we need to + req.Body, err = req.GetBody() + if err != nil { + return nil, err + } + } + + // Actually make the HTTP request with the proper Authorization + return t.originalTransport.RoundTrip(req) +} diff --git a/lib/netext/httpext/error_codes.go b/lib/netext/httpext/error_codes.go index 18d441780f5..d71f0318d1a 100644 --- a/lib/netext/httpext/error_codes.go +++ b/lib/netext/httpext/error_codes.go @@ -35,6 +35,10 @@ import ( "golang.org/x/net/http2" ) +// TODO: maybe rename the type errorCode, so we can have errCode variables? and +// also the constants would probably be better of if `ErrorCode` was a prefix, +// not a suffix - they would be much easier for auto-autocompletion at least... + type errCode uint32 const ( @@ -72,6 +76,10 @@ const ( // HTTP2 Connection errors unknownHTTP2ConnectionErrorCode errCode = 1650 // errors till 1651 + 13 are other HTTP2 Connection errors with a specific errCode + + // Custom k6 content errors, i.e. when the magic fails + //defaultContentError errCode = 1700 // reserved for future use + responseDecompressionErrorCode errCode = 1701 ) const ( @@ -99,6 +107,8 @@ func http2ErrCodeOffset(code http2.ErrCode) errCode { // errorCodeForError returns the errorCode and a specific error message for given error. func errorCodeForError(err error) (errCode, string) { switch e := errors.Cause(err).(type) { + case K6Error: + return e.Code, e.Message case *net.DNSError: switch e.Err { case "no such host": // defined as private in the go stdlib @@ -170,3 +180,27 @@ func errorCodeForError(err error) (errCode, string) { return defaultErrorCode, err.Error() } } + +// K6Error is a helper struct that enhances Go errors with custom k6-specific +// error-codes and more user-readable error messages. +type K6Error struct { + Code errCode + Message string + OriginalError error +} + +// NewK6Error is the constructor for K6Error +func NewK6Error(code errCode, msg string, originalErr error) K6Error { + return K6Error{code, msg, originalErr} +} + +// Error implements the `error` interface, so K6Errors are normal Go errors. +func (k6Err K6Error) Error() string { + return k6Err.Message +} + +// Unwrap implements the `xerrors.Wrapper` interface, so K6Errors are a bit +// future-proof Go 2 errors. +func (k6Err K6Error) Unwrap() error { + return k6Err.OriginalError +} diff --git a/lib/netext/httpext/httpdebug_transport.go b/lib/netext/httpext/httpdebug_transport.go new file mode 100644 index 00000000000..7441aeaf8fa --- /dev/null +++ b/lib/netext/httpext/httpdebug_transport.go @@ -0,0 +1,66 @@ +/* + * + * k6 - a next-generation load testing tool + * Copyright (C) 2019 Load Impact + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +package httpext + +import ( + "fmt" + "net/http" + "net/http/httputil" + + log "github.com/sirupsen/logrus" +) + +type httpDebugTransport struct { + //TODO: get the state and log to its Logger + originalTransport http.RoundTripper + httpDebugOption string +} + +// RoundTrip prints passing HTTP requests and received responses +// +// TODO: massively improve this, because the printed information can be wrong: +// - https://github.com/loadimpact/k6/issues/986 +// - https://github.com/loadimpact/k6/issues/1042 +// - https://github.com/loadimpact/k6/issues/774 +func (t httpDebugTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.debugRequest(req) + resp, err := t.originalTransport.RoundTrip(req) + t.debugResponse(resp) + return resp, err +} + +func (t httpDebugTransport) debugRequest(req *http.Request) { + dump, err := httputil.DumpRequestOut(req, t.httpDebugOption == "full") + if err != nil { + log.Fatal(err) //TODO: fix... + } + fmt.Printf("Request:\n%s\n", dump) //TODO: fix... +} + +func (t httpDebugTransport) debugResponse(res *http.Response) { + if res != nil { + dump, err := httputil.DumpResponse(res, t.httpDebugOption == "full") + if err != nil { + log.Fatal(err) //TODO: fix... + } + fmt.Printf("Response:\n%s\n", dump) //TODO: fix... + } +} diff --git a/lib/netext/httpext/request.go b/lib/netext/httpext/request.go index 6c12ed029d4..f9630ed5555 100644 --- a/lib/netext/httpext/request.go +++ b/lib/netext/httpext/request.go @@ -31,14 +31,12 @@ import ( "net" "net/http" "net/http/cookiejar" - "net/http/httputil" "net/url" "strconv" "strings" "time" ntlmssp "github.com/Azure/go-ntlmssp" - digest "github.com/Soontao/goHttpDigestClient" "github.com/andybalholm/brotli" "github.com/klauspost/compress/zstd" "github.com/loadimpact/k6/lib" @@ -193,6 +191,43 @@ func compressBody(algos []CompressionType, body io.ReadCloser) (*bytes.Buffer, s return buf, contentEncoding, body.Close() } +//nolint:gochecknoglobals +var decompressionErrors = [...]error{ + zlib.ErrChecksum, zlib.ErrDictionary, zlib.ErrHeader, + gzip.ErrChecksum, gzip.ErrHeader, + //TODO: handle brotli errors - currently unexported + zstd.ErrReservedBlockType, zstd.ErrCompressedSizeTooBig, zstd.ErrBlockTooSmall, zstd.ErrMagicMismatch, + zstd.ErrWindowSizeExceeded, zstd.ErrWindowSizeTooSmall, zstd.ErrDecoderSizeExceeded, zstd.ErrUnknownDictionary, + zstd.ErrFrameSizeExceeded, zstd.ErrCRCMismatch, zstd.ErrDecoderClosed, +} + +func newDecompressionError(originalErr error) K6Error { + return NewK6Error( + responseDecompressionErrorCode, + fmt.Sprintf("error decompressing response body (%s)", originalErr.Error()), + originalErr, + ) +} + +func wrapDecompressionError(err error) error { + if err == nil { + return nil + } + + // TODO: something more optimized? for example, we won't get zstd errors if + // we don't use it... maybe the code that builds the decompression readers + // could also add an appropriate error-wrapper layer? + for _, decErr := range decompressionErrors { + if err == decErr { + return newDecompressionError(err) + } + } + if strings.HasPrefix(err.Error(), "brotli: ") { //TODO: submit an upstream patch and fix... + return newDecompressionError(err) + } + return err +} + func readResponseBody( state *lib.State, respType ResponseType, resp *http.Response, respErr error, ) (interface{}, error) { @@ -220,30 +255,30 @@ func readResponseBody( // Transparently decompress the body if it's has a content-encoding we // support. If not, simply return it as it is. contentEncoding := strings.TrimSpace(resp.Header.Get("Content-Encoding")) + //TODO: support stacked compressions, e.g. `deflate, gzip` if compression, err := CompressionTypeString(contentEncoding); err == nil { - var decoder io.ReadCloser + var decoder io.Reader + var err error switch compression { case CompressionTypeDeflate: - decoder, respErr = zlib.NewReader(resp.Body) - rc = &readCloser{decoder} + decoder, err = zlib.NewReader(resp.Body) case CompressionTypeGzip: - decoder, respErr = gzip.NewReader(resp.Body) - rc = &readCloser{decoder} + decoder, err = gzip.NewReader(resp.Body) case CompressionTypeZstd: - var zstdecoder *zstd.Decoder - zstdecoder, respErr = zstd.NewReader(resp.Body) - rc = &readCloser{zstdecoder} + decoder, err = zstd.NewReader(resp.Body) case CompressionTypeBr: - var brdecoder *brotli.Reader - brdecoder = brotli.NewReader(resp.Body) - rc = &readCloser{brdecoder} + decoder = brotli.NewReader(resp.Body) default: // We have not implemented a compression ... :( - respErr = fmt.Errorf( - "unsupported compression type %s for decompression - this is a bug in k6, please report it", + err = fmt.Errorf( + "unsupported compression type %s - this is a bug in k6, please report it", compression, ) } + if err != nil { + return nil, newDecompressionError(err) + } + rc = &readCloser{decoder} } buf := state.BPool.Get() @@ -251,11 +286,12 @@ func readResponseBody( buf.Reset() _, err := io.Copy(buf, rc.Reader) if err != nil { - respErr = err + respErr = wrapDecompressionError(err) } + err = rc.Close() - if err != nil { - respErr = err + if err != nil && respErr == nil { // Don't overwrite previous errors + respErr = wrapDecompressionError(err) } var result interface{} @@ -276,6 +312,7 @@ func readResponseBody( return result, respErr } +//TODO: move as a response method? or constructor? func updateK6Response(k6Response *Response, finishedReq *finishedRequest) { k6Response.ErrorCode = int(finishedReq.errorCode) k6Response.Error = finishedReq.errorMsg @@ -394,19 +431,26 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error tracerTransport := newTransport(state, tags) var transport http.RoundTripper = tracerTransport - if preq.Auth == "ntlm" { - transport = ntlmssp.Negotiator{ - RoundTripper: tracerTransport, + + if state.Options.HttpDebug.String != "" { + transport = httpDebugTransport{ + originalTransport: transport, + httpDebugOption: state.Options.HttpDebug.String, } } + if preq.Auth == "digest" { + transport = digestTransport{originalTransport: transport} + } else if preq.Auth == "ntlm" { + transport = ntlmssp.Negotiator{RoundTripper: transport} + } + 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() - debugResponse(state, req.Response, "RedirectResponse") // Update active jar with cookies found in "Set-Cookie" header(s) of redirect response if preq.ActiveJar != nil { @@ -429,67 +473,15 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error } return http.ErrUseLastResponse } - debugRequest(state, req, "RedirectRequest") return nil }, } - // if digest authentication option is passed, make an initial request - // to get the authentication params to compute the authorization header - if preq.Auth == "digest" { - // TODO: fix - this is very broken! we're always making 2 HTTP requests - // when digest authentication is enabled... we should refactor it as a - // separate transport, like how NTLM auth works - // - // Github issue: https://github.com/loadimpact/k6/issues/800 - username := preq.URL.u.User.Username() - password, _ := preq.URL.u.User.Password() - - // removing user from URL to avoid sending the authorization header fo basic auth - preq.Req.URL.User = nil - - debugRequest(state, preq.Req, "DigestRequest") - res, err := client.Do(preq.Req.WithContext(ctx)) - debugResponse(state, res, "DigestResponse") - body, err := readResponseBody(state, ResponseTypeText, res, err) - finishedReq := tracerTransport.processLastSavedRequest() - if finishedReq != nil { - resp.ErrorCode = int(finishedReq.errorCode) - resp.Error = finishedReq.errorMsg - } - - if err != nil { - // Do *not* log errors about the contex being cancelled. - select { - case <-ctx.Done(): - default: - state.Logger.WithField("error", err).Warn("Digest request failed") - } - - // In case we have an error but resp.Error is not set it means the error is not from - // the transport. For all such errors currently we just return them as if throw is true - if preq.Throw || resp.Error == "" { - return nil, err - } - - return resp, nil - } - - if res.StatusCode == http.StatusUnauthorized { - challenge := digest.GetChallengeFromHeader(&res.Header) - challenge.ComputeResponse(preq.Req.Method, preq.Req.URL.RequestURI(), body.(string), username, password) - authorization := challenge.ToAuthorizationStr() - preq.Req.Header.Set(digest.KEY_AUTHORIZATION, authorization) - } - } - - debugRequest(state, preq.Req, "Request") mreq := preq.Req.WithContext(ctx) res, resErr := client.Do(mreq) - debugResponse(state, res, "Response") resp.Body, resErr = readResponseBody(state, preq.ResponseType, res, resErr) - finishedReq := tracerTransport.processLastSavedRequest() + finishedReq := tracerTransport.processLastSavedRequest(wrapDecompressionError(resErr)) if finishedReq != nil { updateK6Response(resp, finishedReq) } @@ -538,9 +530,7 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error state.Logger.WithField("error", resErr).Warn("Request Failed") } - // In case we have an error but resp.Error is not set it means the error is not from - // the transport. For all such errors currently we just return them as if throw is true - if preq.Throw || resp.Error == "" { + if preq.Throw { return nil, resErr } } @@ -564,16 +554,3 @@ func SetRequestCookies(req *http.Request, jar *cookiejar.Jar, reqCookies map[str } } } - -func debugRequest(state *lib.State, req *http.Request, description string) { - if state.Options.HttpDebug.String != "" { - dump, err := httputil.DumpRequestOut(req, state.Options.HttpDebug.String == "full") - if err != nil { - log.Fatal(err) - } - logDump(description, dump) - } -} -func logDump(description string, dump []byte) { - fmt.Printf("%s:\n%s\n", description, dump) -} diff --git a/lib/netext/httpext/response.go b/lib/netext/httpext/response.go index 0cb9a83a517..c9151b2d459 100644 --- a/lib/netext/httpext/response.go +++ b/lib/netext/httpext/response.go @@ -24,13 +24,9 @@ import ( "context" "crypto/tls" "encoding/json" - "net/http" - "net/http/httputil" - "github.com/loadimpact/k6/lib" "github.com/loadimpact/k6/lib/netext" "github.com/pkg/errors" - log "github.com/sirupsen/logrus" "github.com/tidwall/gjson" ) @@ -115,16 +111,6 @@ func (res *Response) GetCtx() context.Context { return res.ctx } -func debugResponse(state *lib.State, res *http.Response, description string) { - if state.Options.HttpDebug.String != "" && res != nil { - dump, err := httputil.DumpResponse(res, state.Options.HttpDebug.String == "full") - if err != nil { - log.Fatal(err) - } - logDump(description, dump) - } -} - // JSON parses the body of a response as json and returns it to the goja VM func (res *Response) JSON(selector ...string) (interface{}, error) { hasSelector := len(selector) > 0 diff --git a/lib/netext/httpext/transport.go b/lib/netext/httpext/transport.go index 8a06d8a9dd2..afb8619b400 100644 --- a/lib/netext/httpext/transport.go +++ b/lib/netext/httpext/transport.go @@ -162,13 +162,20 @@ func (t *transport) saveCurrentRequest(currentRequest *unfinishedRequest) { } } -func (t *transport) processLastSavedRequest() *finishedRequest { +func (t *transport) processLastSavedRequest(lastErr error) *finishedRequest { t.lastRequestLock.Lock() unprocessedRequest := t.lastRequest t.lastRequest = nil t.lastRequestLock.Unlock() if unprocessedRequest != nil { + // We don't want to overwrite any previous errors, but if there were + // none and we (i.e. the MakeRequest() function) have one, save it + // before we emit the metrics. + if unprocessedRequest.err == nil && lastErr != nil { + unprocessedRequest.err = lastErr + } + return t.measureAndEmitMetrics(unprocessedRequest) } return nil @@ -176,7 +183,7 @@ func (t *transport) processLastSavedRequest() *finishedRequest { // RoundTrip is the implementation of http.RoundTripper func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) { - t.processLastSavedRequest() + t.processLastSavedRequest(nil) ctx := req.Context() tracer := &Tracer{}