x/sync/semaphore: skipping acquisition if the context is done is a breaking change

[baganokodo2022]

Go version

go version go1.21.1 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xinyu.liu/Library/Caches/go-build'
GOENV='/Users/xinyu.liu/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS='-count=1'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xinyu.liu/go/pkg/mod'
GONOPROXY=''
GONOSUMDB='github.cbhq.net'
GOOS='darwin'
GOPATH='/Users/xinyu.liu/go'
GOPRIVATE=''
GOPROXY='https://gomodules.cbhq.net/'
GOROOT='/opt/homebrew/opt/go/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/opt/go/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/xinyu.liu/src/streaming-sdk/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/hh/kbt1yk9d3y56yxzxr382jth80000gn/T/go-build912487987=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

The PR below introduced a breaking change - a regression, and broke our services deployed in production environments.
golang/sync@14be23e

What did you see happen?

We explicitly make use of a zero-timeout context,

ctx, cancel := context.WithTimeout(context.Background(), 0)
defer cancel()

and pass the ctx to sem.Acquire(ctx, 1) to minimize the wait time on a critical path. The goal is to avoid latencies causing by the wait timeouts due to upstream back pressure.

For instance, we use sem to control in-flight messages sent to Kafka. When Kafka brokers experience back pressure, we would like the call to sem.Acquire failed immediately to unblock the main thread, otherwise, it would introduce extra latencies to the critical, user-facing threads.

With this change in the PR, ctx with 0 timeout immediately returns a context cancelled error from sem.Acquire(ctx, 1) even if the semaphore still has a full capacity.

What did you expect to see?

Passing ctx with 0 timeout to sem.Acquire(ctx, int) should successfully acquire the semaphore counts as long as there are still capacity immediately available on the semaphore.

[ianlancetaylor]

The change in question is https://go.dev/cl/536275, written to address issue #63615. CC @szuecs @zephyrtronium

[ianlancetaylor]

From the description of your code, it sounds like you should be using the TryAcquire method rather than the Acquire method.

[baganokodo2022]

In that case, I need to figure out the timeout on the context, and call TryAcquire when the timeout is set to zero, and call Acquire when the timeout is not zero.

Since the timeout is set by client programs, it doesn't seem to be viable. I have to say, the vast majority of our clients don't set zero timeout on the context, while there are still very few performing fire-and-forget message publishing and expecting minimum latencies on back pressure.

Thoughts?

[ianlancetaylor]

You can call the context.Context.Deadline method to find out whether the context has timed out.

[ruyi789]

The 0 timeout that triggers done depends only on the context, so you should fix the context

[neild]

I think there's a reasonable argument to be made that CL 536275 was not a backwards-compatible change.

There are three things a function can do when passed a pre-canceled context: Succeed, fail, or pick one at random. Weighted.Acquire was documented as possibly doing any of these ("If ctx is already done, Acquire may still succeed..."), but the implementation was to consistently succeed. CL 536275 changed it to consistently fail. The new behavior is consistent with the documentation, but so was the previous behavior. I also don't think there's a clear right choice on always-fail vs. always-succeed here.

In general, we try to avoid making substantive changes that may cause existing programs to break, even if the documentation permits the change, except when the change is a clear improvement. I'm not convinced this change meets that bar.

That said, the change was made about nine months ago. Reverting it at this time would be just more churn and opportunity for breakage.