x/sync/semaphore: acquire should fail if context is canceled or timed out

[szuecs]

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

Issue is also reproducibe in https://go.dev/play/p/CNcJihWeMMU

$ go version
go version go1.21.3 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output

$ go env
GO111MODULE='on'
GOARCH='amd64'
GOBIN='/home/sszuecs/go/bin'
GOCACHE='/home/sszuecs/.cache/go-build'
GOENV='/home/sszuecs/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/sszuecs/go/pkg/mod'
GONOPROXY='github.bus.zalan.do'
GONOSUMDB='github.bus.zalan.do'
GOOS='linux'
GOPATH='/home/sszuecs/go'
GOPRIVATE='github.bus.zalan.do'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/share/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/share/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.3'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/sszuecs/tmp/go/semaphore/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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3306102333=/tmp/go-build -gno-r
ecord-gcc-switches'

What did you do?

Reproducible error:
https://go.dev/play/p/CNcJihWeMMU

What did you expect to see?

no error printed

What did you see instead?

2009/11/10 23:00:00 acquire expected to fail on done context

[ianlancetaylor]

CC @jba

[bcmills]

In general a Go API that accepts a Context can either succeed or fail if the context is already canceled, as long as it doesn't block in the process.

That said, I agree that it would be reasonable for Acquire to provide the stronger invariant that if the Context is already done before the method is called, the error is always non-nil.

[szuecs]

@bcmills I found it with @AlexanderYastrebov when I tried to use it with context.WithTimeout.

For the records, you can use https://go.dev/play/p/wgsWdy8mGGo to handle the timeout case.
For me it was a bit unexpected to test in the good case the timeout (/cancel) case.

[zephyrtronium]

The documentation on Acquire states that this is allowed behavior. Does changing this need to go through proposal review?

[gopherbot]

Change https://go.dev/cl/536275 mentions this issue: semaphore: cancel acquisition with a done context

[zephyrtronium]

To be clear, is the desired behavior (in terms of the language of the Go memory model) "if ctx closing happens before the call to Acquire returns, Acquire never acquires the semaphore," or is it sufficient to say, "if ctx closing happens before the call to Acquire begins, Acquire never acquires the semaphore?" The latter is easier and probably cheaper to implement.

[bcmills]

@zephyrtronium, I think the only reasonable guarantee we can make is if ctx is already canceled at the start of the call. (If it is canceled partway through, then there isn't a happens-before relationship between acquiring the semaphore and canceling the context — canceling a context.WithCancel is not guaranteed to be synchronous.)

[zephyrtronium]

Correct me if I'm wrong, but I think it is possible to construct a happens-before relation with (Acquire starts) <= (context canceled) <= (Acquire completes):

1. Goroutines A and B share an empty semaphore of size 2, and A has the ability to cancel the context B will use to acquire the semaphore.
2. A acquires weight 1, then signals to (or creates) B.
3. B calls Acquire(ctx, 2).
4. A continually calls TryAcquire(1) followed by Release(1) when it succeeds, so that A synchronizes with B's Acquire.
5. A cancels B's context and then calls Release(1).

sem := semaphore.NewWeighted(2)
ctx, cancel := context.WithCancel(context.Background())
sem.TryAcquire(1)
go sem.Acquire(ctx, 2)
for sem.TryAcquire(1) {
    sem.Release(1)
}
cancel()
sem.Release(1)

With the current implementation or if we choose the closed-before-begins option (implemented with a minor adjustment to CL 536275's current patch set), the semaphore may have either 0 or 2 tokens held at the end of this sequence. I believe that having Acquire immediately release its acquisition if it finds both context canceled and a ready signal is sufficient to implement the closed-before-returns option and leave the semaphore always at 0 tokens held.

To me, the question is more about how much work will happen after the semaphore is acquired with a canceled context. Or more precisely, does Acquire really need to be the first point where the context being done causes the caller to quit its work? Maybe the situation that caused this issue for @szuecs in the first place would help answer that question.