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: Transport.CancelRequest not always called if RoundTrip returns promptly when a request times out #34552

Closed
axw opened this issue Sep 26, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@axw
Copy link
Contributor

axw commented Sep 26, 2019

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

$ go version
go version devel +770a13546e Thu Sep 26 06:48:55 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/andrew/.cache/go-build"
GOENV="/home/andrew/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/andrew/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/andrew/.gimme/versions/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/andrew/.gimme/versions/go/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/andrew/go/src/go.elastic.co/apm/module/apmhttp/go.mod"
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-build282289598=/tmp/go-build -gno-record-gcc-switches"

What did you do?

The tests for my project, go.elastic.co/apm, started failing when running against go tip.

I strongly suspect 7fc2625, since it started happening today. The issue can be reproduced with https://play.golang.org/p/MQepDRMdcjL

What did you expect to see?

The program should run and exit cleanly.

What did you see instead?

The program runs for ~10 seconds and panics, due to CancelRequest not being called.

Note that this doesn't happen every time, hence the loop. If I increase the timeout to 50ms it becomes less frequent, but still fails occasionally.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 26, 2019
@ALTree ALTree added this to the Go1.14 milestone Sep 26, 2019
@bcmills
Copy link
Contributor

bcmills commented Sep 26, 2019

due to CancelRequest not being called.

Why do you expect CancelRequest to be called?

Per its documentation:

Deprecated: Use Request.WithContext to create a request with a cancelable context instead. CancelRequest cannot cancel HTTP/2 requests.

That seems to also imply that Transport implementations should use req.Context() rather than only relying on CancelRequest.

A version of your test program modified to do so (https://play.golang.org/p/7ZZRvEpy9uJ) seems to confirm that in all cases, either CancelRequest was called, or req.Context().Done() was closed before the end of the RoundTrip call.

@bcmills bcmills changed the title net/http: Transport.CancelRequest is racy again net/http: Transport.CancelRequest not always called when a request times out Sep 26, 2019
@bcmills
Copy link
Contributor

bcmills commented Sep 26, 2019

CC @bradfitz to confirm, but I'm pretty sure this is working as designed.

An additional test confirms that CancelRequest is always called if RoundTrip blocks:
https://play.golang.org/p/1UjWdVrhdgh

@bcmills bcmills changed the title net/http: Transport.CancelRequest not always called when a request times out net/http: Transport.CancelRequest not always called if RoundTrip returns promptly when a request times out Sep 26, 2019
@bcmills bcmills closed this as completed Sep 26, 2019
@bradfitz
Copy link
Contributor

Yup, looks like it's working fine. In the original snippet there was no need for net/http to call CancelRequest because Do had already failed in time.

Thanks for investigating, @bcmills.

@axw
Copy link
Contributor Author

axw commented Sep 27, 2019

Why do you expect CancelRequest to be called?

Because the Client.Timeout docs say it will be :)

The Client cancels requests to the underlying Transport
as if the Request's Context ended.

For compatibility, the Client will also use the deprecated
CancelRequest method on Transport if found. New
RoundTripper implementations should use the Request's Context
for cancellation instead of implementing CancelRequest.

@bcmills for context, I'm writing tracing middleware that wraps others' arbitrarily old and crusty code, harking back from from versions of Go that do not support context. I don't have much of a say about whether they use CancelRequest or Context cancellation.

Perhaps the docs ought to be clarified? Then again, we've probably spent enough time in this dusty corner. I'll change my test to accommodate.

Thanks both for looking into it.

@golang golang locked and limited conversation to collaborators Sep 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants