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: HTTP CONNECT failures should return a more helpful error #38143

Open
elagergren-spideroak opened this issue Mar 29, 2020 · 3 comments
Open

Comments

@elagergren-spideroak
Copy link

@elagergren-spideroak elagergren-spideroak commented Mar 29, 2020

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:

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)
@elagergren-spideroak 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
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 29, 2020

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
Copy link
Author

@elagergren-spideroak elagergren-spideroak commented Mar 30, 2020

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
Copy link

@networkimprov networkimprov commented Aug 19, 2020

@gopherbot remove WaitingForInfo

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
5 participants
You can’t perform that action at this time.