fix(async/unstable-semaphore): bind release in constructor (#7195)

[sanjibani]

Fixes #7195.

Summary

Semaphore#release reads private fields (#head, #count, #max) and
therefore needs its receiver intact. Passing the method as a bare callback
(for example promise.finally(sem.release)) drops the this binding and
crashes with TypeError: Cannot read properties of undefined (reading '#head').
Destructuring (const { release } = sem) has the same hazard.

Fix

Bind release once in the constructor. The bound copy becomes an own
property on the instance, so any direct member access (sem.release,
destructure, spread, higher-order callback) hands callers a function that
still resolves the private state correctly.

constructor(max: number = 1) {
  if (!Number.isInteger(max) || max < 1) {
    throw new TypeError(
      `Cannot create semaphore as 'max' must be a positive integer: received ${max}`,
    );
  }
  this.#count = this.#max = max;
  // Bind methods so they remain safe when passed unbound to higher-order
  // callbacks such as `.finally(sem.release)` (issue #7195).
  this.release = this.release.bind(this);
}

Tests

Added two regression tests next to the existing unstable_semaphore_test.ts
suite. Both use the original .finally(sem.release) and const { release }
shapes from the bug report:

• Semaphore.release() can be passed unbound to .finally()
• Semaphore.release() can be destructured

Both tests would throw the original TypeError against the unpatched code.

Notes

The module is marked @experimental / UNSTABLE, so the API surface is
free to evolve. Binding one method does not change observable behaviour
for existing call sites that invoke sem.release() directly — they
continue to dispatch through the bound own property.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[bartlomieju]

Thanks for picking this up — and the bug in #7195 is real, in fact worse than the issue suggests. I reproduced it locally: const { release } = sem; release() throws TypeError: Cannot read properties of undefined (reading '#head'). The issue's "sometimes works" is misleading; it never works, it just looks like it does when nothing is queued and the resulting rejection gets swallowed inside an unawaited .finally() chain.

So the problem is worth fixing. Two things need resolving before this can land.

First, CI is red, and the failing test is the one meant to demonstrate the fix — details inline.

Second, and this is a call for the team rather than something you did wrong: there is currently no .bind(this) anywhere in any @std class, so this would set a precedent. It also fixes only release, while acquire and tryAcquire touch the same private fields and still break when destructured — so a user who learns "semaphore methods are safe to pass around" from release gets bitten by the other two. I'd like a maintainer to weigh in on whether we want constructor binding applied consistently across the class, or would rather document () => sem.release() at the call site. I don't want you to do a lot of rework before that's settled.

Smaller: the PR title scope needs to be fix(async/unstable) rather than fix(async/unstable-semaphore) for the title check to pass, and the CLA still needs signing.

[bartlomieju]

This is what's red-lining CI, and unfortunately the test can't validate the fix even once it stops hanging.

assertBlocks(promise, releaser) expects releaser to be the thing that unblocks promise. Here the releaser is () => {}, so nothing ever releases the permit: the second acquire() never settles, and the test dies with Promise resolution is still pending but the event loop has already resolved. I confirmed this locally against the patched source.

The deeper problem is the shape: .finally(sem.release) only runs after the acquire it is supposed to unblock has already settled, so this arrangement can never exercise the unbound-release path. The test on line 113 (can be destructured) is the one that actually proves the fix — it passes release as the releaser, which is exactly right.

Simplest resolution is to drop this test and keep the destructuring one. If you want .finally coverage specifically, it needs to be a separate permit — acquire, then somePromise.finally(sem.release), then assert a later acquire() becomes unblocked.

[bartlomieju]

Two consequences of binding here that are worth being explicit about, whichever way the design question goes:

1. This creates an enumerable own property, so release now shows up in Object.keys(sem), object spread, and structural comparisons like assertEquals. Two semaphores with identical state will no longer compare equal, since each holds a distinct bound function.
2. It shadows the prototype method, so stub(Semaphore.prototype, "release") no longer affects instances that already exist. Instance-level stub(sem, "release") still works, but anyone mocking via the prototype gets a silent no-op.

Neither is fatal, and binding does keep the method on the prototype (unlike an arrow class property), which is better for deno doc and JSR output. But both deserve a maintainer's eyes before we set the precedent.

[bartlomieju]

Nit: the comment says "Bind methods" (plural) but only one method is bound. That plural is a decent hint that the fix wants to cover acquire and tryAcquire too — they read the same private fields and fail identically when destructured.