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: Client.Do documentation is incorrect about *url.Error.Timeout #46402

Closed
perillo opened this issue May 26, 2021 · 5 comments
Closed
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@perillo
Copy link
Contributor

perillo commented May 26, 2021

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

$ go version
go version go1.16.4 linux/amd64

Does this issue reproduce with the latest release?

Yes.
It also reproduces with go1.17 devel.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/manlio/.local/bin"
GOCACHE="/home/manlio/.cache/go-build"
GOENV="/home/manlio/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE="*.local"
GOMODCACHE="/home/manlio/.local/lib/go/pkg/mod"
GONOPROXY=""
GONOSUMDB="*.local"
GOOS="linux"
GOPATH="/home/manlio/.local/lib/go:/home/manlio/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.4"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1653706074=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.16.4 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.16.4
uname -sr: Linux 5.12.5-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) release release version 2.33.
gdb --version: GNU gdb (GDB) 10.2

What did you do?

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

What did you expect to see?

timed out: true

As documented by https://golang.org/pkg/net/http/#Client.Do:

Any returned error will be of type *url.Error. The url.Error value's Timeout method will report true if request timed out or was canceled.

What did you see instead?

timed out: false

@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label May 26, 2021
@mknyszek mknyszek added this to the Backlog milestone May 26, 2021
@mknyszek
Copy link
Contributor

If this is just a documentation fix, this can land any time. I haven't read deeply into this, but maybe you're up for contributing the doc fix? :) See https://golang.org/doc/contribute.

CC @neild

@perillo
Copy link
Contributor Author

perillo commented May 26, 2021

Looking at the source code, the implementation does not check if Client.send returns context.Canceled. So it is either the documentation incorrect or the implementation incorrect.

Also, I think that the TODO in https://go.googlesource.com/go/+/refs/heads/master/src/net/http/client.go#732 is wrong. The send function seems to returns err == context.Canceled and didTimeout() == false when the context is canceled.

@neild
Copy link
Contributor

neild commented May 26, 2021

I was going to say that we should drop the mention of the Timeout method in favor of errors.Is(err, context.DeadlineExceeded) or errors.Is(err, os.DeadlineExceeded), but those are apparently different errors and I'm not 100% certain that net/http consistently returns one or the other.

We should make sure that it's possible to test for timeouts with a single sentinel error and document that.

@perillo
Copy link
Contributor Author

perillo commented May 27, 2021

According to #33545, the problem is with the documentation. https://golang.org/cl/200798 also locked-in the current behavior with a test.

The problem is that #33545 only fixed the documentation of the Client.Get method, forgetting(?) the Get function and Client.Do method.

@gopherbot
Copy link

Change https://golang.org/cl/323089 mentions this issue: net/http: fix Get and Client.Get docs on request cancelation

@golang golang locked and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants