Skip to content

Semaphore over-release guard, asymmetric visibility, and a few other small improvements #1

@jeffreybulanadi

Description

@jeffreybulanadi

Going through the source, a few things stood out worth discussing before a PR.

1. Semaphore backed property hooks add no value over asymmetric visibility

$available and $capacity use backed property hooks that do nothing but return the backing store:

public int $available {
    get => $this->available;
}

PHP 8.4 public private(set) is the idiomatic way to express this - publicly readable, privately writable - without hook dispatch overhead on every read:

public private(set) int $available;
public private(set) int $capacity;

Mutex::$queueLength is different since its hook computes from $this->queue, so the hook there is justified.

2. Semaphore::release() does not guard against over-release when the queue is non-empty

The capacity check only fires in the no-queue path:

if (\count($this->queue) === 0) {
    if ($this->available >= $this->capacity) {
        throw new \LogicException(...);
    }
    // ...
}
// queue non-empty path: available++ with no capacity guard

If a caller releases more permits than they acquired while waiters are queued, $available temporarily exceeds capacity. The exception eventually fires once the queue drains, but the semaphore is in an inconsistent state between those calls. The guard should apply unconditionally before the increment:

if ($this->available >= $this->capacity) {
    throw new \LogicException('Cannot release more permits than semaphore capacity');
}

3. Missing final on both concrete classes

Both Mutex and Semaphore are subclassable. Their synchronization guarantees depend entirely on private state ($locked, $queue, $available). A subclass cannot override the queueing logic, so subclassing either class can only produce broken variants. Marking them final closes that door and is consistent with how other internal Hibla classes are treated.

4. tryAcquire() bypasses the waiter queue without documenting the starvation risk

tryAcquire() intentionally skips the queue, which is fine for its use case. But if a high-frequency caller loops on tryAcquire(), waiters in the queue can starve indefinitely. Worth noting in the docblock so callers understand when it is and is not appropriate.

5. No tryAcquireMany() to pair with acquireMany()

The interface has acquire() / tryAcquire() and acquireMany() but no tryAcquireMany(int $permits). If non-blocking multi-permit acquisition is out of scope that is fine, but the asymmetry is noticeable.

Happy to put together a PR for any or all of these if the direction looks right.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions