Skip to content

fix(bulkhead): release permits granted to abandoned waiters - #400

Open
Empatixx wants to merge 1 commit into
failsafe-lib:masterfrom
Empatixx:fix/bulkhead-permit-leak-on-acquire-timeout
Open

fix(bulkhead): release permits granted to abandoned waiters#400
Empatixx wants to merge 1 commit into
failsafe-lib:masterfrom
Empatixx:fix/bulkhead-permit-leak-on-acquire-timeout

Conversation

@Empatixx

Copy link
Copy Markdown

Summary

Bulkhead#tryAcquirePermit(Duration) permanently destroys a permit every time it gives up. Once enough acquisitions have timed out, the bulkhead reaches zero capacity and never recovers for the lifetime of the instance — every subsequent acquisition fails even when nothing is executing.

Fixes #393.

Root cause

tryAcquirePermit(Duration) enqueues a waiter through acquirePermitAsync(), but on timeout it just returns:

} catch (CancellationException | ExecutionException | TimeoutException e) {
  return false;   // the waiter stays in `futures`
}

The abandoned waiter is still first in the queue, so the next releasePermit() hands it the permit:

public synchronized void releasePermit() {
  if (permits < maxPermits) {
    permits += 1;
    CompletableFuture<Void> future = futures.pollFirst();
    if (future != null){
      permits -= 1;             // handed to the abandoned waiter
      future.complete(null);    // nobody is reading it
    }
  }
}

Nobody will ever call releasePermit() for that grant, so the permit is gone for good.

Reproducer

No test framework — just failsafe-3.3.2.jar on the classpath. Eight concurrent callers time out against a bulkhead of capacity 8:

import dev.failsafe.Bulkhead;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;

public class BulkheadLeak {
  static final int MAX = 8;

  public static void main(String[] args) throws Exception {
    Bulkhead<Void> bulkhead = Bulkhead.<Void>builder(MAX).build();
    System.out.println("initial capacity = " + capacity(bulkhead));

    for (int round = 1; round <= 5; round++) {
      for (int i = 0; i < MAX; i++) bulkhead.tryAcquirePermit();

      List<Thread> waiters = new ArrayList<>();
      for (int i = 0; i < MAX; i++) {
        Thread t = new Thread(() -> {
          try { bulkhead.tryAcquirePermit(Duration.ofMillis(50)); }
          catch (InterruptedException ignored) { Thread.currentThread().interrupt(); }
        });
        t.start();
        waiters.add(t);
      }
      for (Thread t : waiters) t.join();

      for (int i = 0; i < MAX; i++) bulkhead.releasePermit();
      System.out.println("after round " + round + " = " + capacity(bulkhead));
    }
  }

  static int capacity(Bulkhead<Void> b) {
    int n = 0;
    while (b.tryAcquirePermit()) n++;
    for (int i = 0; i < n; i++) b.releasePermit();
    return n;
  }
}

On 3.3.2 and on current master:

initial capacity = 8
after round 1 = 0
after round 2 = 0
after round 3 = 0
after round 4 = 0
after round 5 = 0

With this patch it stays at 8 every round.

A single burst is enough — the capacity does not decay gradually, it collapses.

Why this is critical

A Bulkhead is normally built once and reused for the life of the process. Once wedged it cannot be recovered through the public API: releasePermit() only feeds the next zombie, and there is no reset. The only remedy is to recreate the bulkhead, which callers have no way of knowing they need to do.

The failure is also silent and delayed: it is triggered by a brief overload spike, but it manifests afterwards, when load is back to normal and nothing is in flight. The symptom is a component that permanently rejects all work while looking idle.

We hit this in production. One traffic burst briefly pushed concurrent requests past the configured 64 permits; from that point on every request through that bulkhead failed at the acquire timeout, with zero threads actually executing, until the process was restarted.

Fix

Complete the abandoned waiter, which removes it from the queue (FutureLinkedList unlinks on completion). If completing fails, releasePermit() had already granted it a permit, so release it back rather than lose it — this closes the race between the timeout and a concurrent release.

The same leak applied to InterruptedException, in both acquirePermit() and tryAcquirePermit(Duration); both are handled.

Tests

Added BulkheadImplTest with two cases. Against master they fail:

BulkheadImplTest.shouldRetainCapacityOverRepeatedAcquireTimeouts:55 capacity after round 1 expected [4] but found [3]
BulkheadImplTest.shouldRetainPermitAfterAcquireTimesOut:41 the released permit must be available again expected [true] but found [false]

With the fix they pass, and the full core suite is green (308 tests, 0 failures).

tryAcquirePermit(Duration) enqueues a waiter via acquirePermitAsync() but, on
timeout, returns false without removing it from the queue. The next
releasePermit() polls that abandoned waiter, decrements permits and completes a
future nobody reads, so the permit is destroyed rather than returned.

Capacity therefore ratchets downwards and never recovers. When more callers time
out than there are permits, the bulkhead drops to zero in a single burst and
every subsequent acquisition fails for the lifetime of the instance.

Completing the abandoned waiter removes it from the queue (FutureLinkedList
unlinks on completion). If completing fails, releasePermit() had already granted
it a permit, which is then released so it is not lost.

The same leak applied to InterruptedException in both acquirePermit() and
tryAcquirePermit(Duration).

Fixes failsafe-lib#393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bulkhead(Executor) does not always release permits

2 participants