net/http: HTTP CONNECT failures should return a more helpful error #38143

elagergren-spideroak · 2020-03-29T03:59:18Z

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

$ go version
go version go1.14.1 darwin/amd64

Does this issue reproduce with the latest release?

Yep

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="..."
GOCACHE="/Users/.../Library/Caches/go-build"
GOENV="/Users/.../Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/.../gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="..."
CXX="clang++"
CGO_ENABLED="1"
GOMOD="..."
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/2b/74fz3jhd4wz4vnbf4z7ywzww0000gp/T/go-build135874009=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/CsZfrDwLeFb

What did you expect to see?

An error I could inspect.

What did you see instead?

A *url.Error with its Err set to errors.New(...).

In certain environments we use HTTP CONNECT, and on occasion some of the destination servers are temporarily unreachable. When this happens with other proxies, we can inspect the HTTP response (status codes and/or headers) or the error (does Temporary report true?) to determine whether we should backoff and retry the request.

However, net/http inspects the response from the initial CONNECT request and if it's anything other than 200, it returns an generic error constructed from the status code text:

go/src/net/http/transport.go

Line 1622 in 564c76a

if resp.StatusCode != 200 {

This means we're limited to something like

var (
    httpStatusServiceUnavailable = http.StatusText(http.StatusServiceUnavailable)
    ...
)

resp, err := c.Get(...)
if err != nil {
    switch s := err.(*url.Error).Err.Error(); s {
    case httpStatusServiceUnavailable:
        /* backoff then retry */
    ...
    default:
        return err
    }
}
...

An example of a useful error would be something like

type ConnectError struct {
    StatusCode int
    ...
}

func (c *ConnectError) Error() string {
    return StatusText(c.StatusCode)
}

var _ error = (*ConnectError)(nil)

The text was updated successfully, but these errors were encountered:

odeke-em · 2020-03-29T18:00:01Z

Thank you for filing this bug @elagergren-spideroak and great to catch you here!

I can sympathize with this problem, and this code was written almost a decade ago (~9 years ago)
in e0a2c5d, when we just had os.ErrorString, and this package was just young.

Thank you for the suggestion to add ConnectError, but given that I've encountered this problem and I think that there might other places that could use StatusCode and message, perhaps let's make it CodedError which will look like this

type CodedError struct {
    Code int
    Message string
    Method string
}

type (ce *CodedError) Error() string { return ce.Message }

and in usage you can do:

resp, err := c.Get(...)
if err != nil {
     ce, ok := err.(*http.CodedError)
     if !ok {
             return err
     }

     // Otherwise check and deal with backoff + retries.
     ...
}

That type can also be generalized for use in other packages like the net, in which for example we can provide more details on why a Dial failed, but also specify where in the dialling stack it failed, by specifying Method.

One thing we'll need to be aware of, if we add this error is:
a) What are the odds of breaking almost decades long code that switches on an error string? How much code already handles errors by switching on a status or expecting a string as the only detail from invoking err.Error()?

elagergren-spideroak · 2020-03-30T19:18:05Z

For what it's worth, I'm not too concerned about the actual error—ConnectError was just an example. The ticket is about getting any error that I can inspect, not a particular error :)

However, like you mentioned, ensuring backwards compatibility is important, thus why in my example the Error method just returned http.StatusText.

Comparisons with the current error always evaluate to false, so I can't imagine anybody relying on the behavior.

networkimprov · 2020-08-19T07:30:33Z

@gopherbot remove WaitingForInfo

gopherbot · 2021-02-05T11:33:56Z

Change https://golang.org/cl/289929 mentions this issue: net/http: add new error type to be returned on unsuccessful CONNECT request

logandavies181 · 2021-05-13T05:41:48Z

@odeke-em @andybons @ericlagergren Hello, I raised a PR that would address this (and its duplicates) a few months ago but haven't received any feedback on my PR. Could you please help out?

networkimprov · 2021-05-13T06:42:13Z

cc @neild @fraenkel @bradfitz

elagergren-spideroak changed the title ~~net/http: HTTP CONNECT should return a more helpful error~~ net/http: HTTP CONNECT failures should return a more helpful error Mar 29, 2020

andybons added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Mar 30, 2020

andybons added this to the Unplanned milestone Mar 30, 2020

ericlagergren mentioned this issue Aug 19, 2020

proposal: net/http: increase verbosity of unsuccessful proxy CONNECT request #40873

Closed

gopherbot removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 19, 2020

elagergren-spideroak mentioned this issue Feb 5, 2021

proposal: net/http: return error for proxy CONNECT errors #44118

Closed

logandavies181 linked a pull request Feb 5, 2021 that will close this issue

net/http: add new error type to be returned on unsuccessful CONNECT request #44123

Open

aelsabbahy mentioned this issue Aug 26, 2023

https test's dont honor Status+Header if expected as "ok" if failed goss-org/goss#765

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: HTTP CONNECT failures should return a more helpful error #38143

net/http: HTTP CONNECT failures should return a more helpful error #38143

elagergren-spideroak commented Mar 29, 2020 •

edited

odeke-em commented Mar 29, 2020

elagergren-spideroak commented Mar 30, 2020 •

edited

networkimprov commented Aug 19, 2020

gopherbot commented Feb 5, 2021

logandavies181 commented May 13, 2021 •

edited

networkimprov commented May 13, 2021

net/http: HTTP CONNECT failures should return a more helpful error #38143

net/http: HTTP CONNECT failures should return a more helpful error #38143

Comments

elagergren-spideroak commented Mar 29, 2020 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

odeke-em commented Mar 29, 2020

elagergren-spideroak commented Mar 30, 2020 • edited

networkimprov commented Aug 19, 2020

gopherbot commented Feb 5, 2021

logandavies181 commented May 13, 2021 • edited

networkimprov commented May 13, 2021

elagergren-spideroak commented Mar 29, 2020 •

edited

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

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

elagergren-spideroak commented Mar 30, 2020 •

edited

logandavies181 commented May 13, 2021 •

edited