net/http: http client changes context cancel error behaviour in 1.23+

[phanhuyn]

Go version

1.23.7

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/phanhuy1502/Library/Caches/go-build'
GOENV='/Users/phanhuy1502/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/phanhuy1502/dev/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/phanhuy1502/dev/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/phanhuy1502/dev/go/pkg/mod/golang.org/[email protected]'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/phanhuy1502/dev/go/pkg/mod/golang.org/[email protected]/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.7'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/phanhuy1502/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/phanhuy1502/dev/src/playground/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/y3/rvr96zrj7wlcjhp3dsxv_wch0000gn/T/go-build3316156720=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

The following code on a http client returns different error in golang 1.22 and 1.23+.

In 1.22, when context is canceled with a cause, client.Do returns context.Cancelled.

In 1.23, when context is canceled with a cause, client.Do returns context.Caused.

package main

import (
	"context"
	"errors"
	"log"
	"net/http"
)

func main() {
	client := &http.Client{}

	ctx := context.Background()
	ctx, cancel := context.WithCancelCause(ctx)
	context.Canceled

	req, err := http.NewRequestWithContext(ctx, "GET", "https://www.github.com", nil)
	if err != nil {
		log.Fatalf("Error creating request: %v\n", err)
	}

	// cancel the request with a custom cause
	cancel(errors.New("test error"))

	resp, err := client.Do(req)
	if err != nil {
		// 1.22: return context.Canceled
		// 1.23+: return "test error" (the cause)
		log.Fatalf("Error making request: %v\n", err)
	}

	resp.Body.Close()
}

What did you see happen?

In 1.22, when context is canceled with a cause, client.Do returns context.Canceled.
In 1.23, when context is canceled with a cause, client.Do returns the cause of the cancelation (context.Cause(ctx))

I'm suspecting this is related to this code change release in 1.23+.
505000b#diff-f2f92ffe0abe8dd3c833d435c2d859d54380e8e4160af8becab6945395563cfeL594

Same issue with 1.24

What did you expect to see?

The "correct" behaviour is debatable, but this change in behaviour may cause existing code to malfunction. So i'm expecting the http client should return the context.Canceled error if the context is cancelled (or at least, the returning err should satisfy errors.Is(err, context.Canceled) == true)

Perhaps, a errors.Join(context.Canceled, context.Cause(ctx) should be returned here: 505000b#diff-f2f92ffe0abe8dd3c833d435c2d859d54380e8e4160af8becab6945395563cfeL594

In my case, I rely on the err returned by http client to be context.Canceled to implement circuit breaker (if context errors.Is(err, context.Canceled) == true, don't count as an error). The upgrade to 1.23 breaks this behaviour.

[ianlancetaylor]

I'm sorry it breaks your code, but this change in behavior seems like exactly what should happen. The point of adding WithCancelCause is to permit people to cancel contexts with more specific errors. That is only useful if those errors are returned up the stack.

I agree that it makes sense for code that uses WithCancelCause to call the CancelCauseFunc with an error for which errors.Is(err, context.Canceled) returns true, but I don't think it's the job of the net/http package to enforce that as a requirement.

CC @neild

[phanhuyn]

I'm sorry it breaks your code, but this change in behavior seems like exactly what should happen. The point of adding WithCancelCause is to permit people to cancel contexts with more specific errors. That is only useful if those errors are returned up the stack.

I agree that it makes sense for code that uses WithCancelCause to call the CancelCauseFunc with an error for which errors.Is(err, context.Canceled) returns true, but I don't think it's the job of the net/http package to enforce that as a requirement.

CC @neild

Thanks @ianlancetaylor for the quick response. I think we can justify the new behaviour like you mentioned. Though I think the checking of errors.Is(err, context.Canceled) is quite common, e.g. kubernetes, docker, prometheus, containerd - from eyeballing it, some are checking the error from the http client call - so maybe it's worth it to provide compatibility in this sense 🤔 - Or mention this change in the release note.

I agree that it makes sense for code that uses WithCancelCause to call the CancelCauseFunc with an error for which errors.Is(err, context.Canceled) returns true

If the code that uses WithCancelCause should always ensure this condition errors.Is(err, context.Canceled) returns true, would it make sense in the stdlib to make .WithCancelCause to wrap/join the passed in error with context.Canceled?