Main-thread acquireSyncOrFail and withSyncOrFail

[lucacasonato]

Issue originally posted by @bakkot in lucacasonato/proposal-semaphore#1

One of the questions in the readme:

Should the Semaphore have a way to query the number of tokens available?

I think definitely yes.

Moreover I think there should be a way to synchronously acquire a token when one is available, as in

if (semaphore.tokens > 0) {
  using permit = semaphore.acquireSyncOrFail();
  // do work
} else {
  throw "could not do work right now"
}

or

if (semaphore.tokens > 0) {
  semaphore.withSyncOrFail(syncCallback);
}

The await semaphore.acquire() construct is convenient in places which are already async, but not all places can afford to be async.

Unfortunately this pattern fits awkwardly with cross-thread semaphores, because someone can in theory acquire a token between when you check if any are available and when you call acquireSyncOrFail. But as long as acquireSyncOrFail is atomic, that just gives you an error, not a race.

---

Maybe this is what you're getting at with the "try acquire" possibility raised in the readme, if you're suggesting that such a method be synchronous, but I think it's important that it throw rather than returning null. Otherwise it's too easy to write using permit = semaphore.tryAcquire() and not notice that it failed to acquire the token.

[lucacasonato]

@bakkot I did indeed mean a synchronous acquire or return null/throw with "try acquire".

I am not yet convinced of the need to know the current number of available tokens. Specifically, if you have a synchronous acquire method you can do a non racy synchronous acquire, even in multi-agent scenarios, without needing to know the available tokens. I am worried that exposing the current number of available tokens would lead people to write acquiry code that contains TOCTOU races.

- if (semaphore.availableTokens > 0) {
-   semaphore.tryAcquireSync(); // this could throw!
- }
+ try {
+   semaphore.tryAcquireSync();
+ } catch {
+   // failed to acquire, try again later
+ }

I see your point about throwing vs returning null. However, if we do not provide a way to query for available tokens we have to expect that this function could be in some rather hot paths. And throwing is generally much less optimized in engines than null checking. I am also not ready to settle on one or the other here yet.

[bakkot]

I see your point about throwing vs returning null. However, if we do not provide a way to query for available tokens we have to expect that this function could be in some rather hot paths. And throwing is generally much less optimized in engines than null checking. I am also not ready to settle on one or the other here yet.

I feel strongly that avoiding the using permit = semaphore.tryAcquireSync() issue is more important than being able to hit those hot paths. I suppose we could find some other approach, though, such as returning the string "none available" on failure. That said I think throwing is fine; this is not the common case and we don't need to worry as much about optimizing it.

[michaelficarra]

this is not the common case and we don't need to worry as much about optimizing it.

Why is it not the common case? I think it could be the common case in some applications.

[bakkot]

For "common case" instead read "happy path". That is, in the "resource is not available" case you're going to have to go back and wait for someone to release the resource, which is probably going to take a lot longer than whatever overhead there is from the exception.

[lucacasonato]

@bakkot I don't think that is necessarily true. You may want to use this as a "only collect traces for 10 req at a time" mechanism where you check on every incoming request whether you should trace it (based on the governor's capacity)

[bakkot]

It's true that it's possible to write code where the performance cost of the exception would matter, but I really do not expect that to be at all common.