SignalProcessor.executeWithSemaphore ignores tryAcquire result and runs the critical section unguarded on timeout
- Dominant language
- HTML
- Stars
- 6
- Forks
- 8
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 1
Description
### Bug report
`SignalProcessor.executeWithSemaphore(...)` discards the boolean result of `Semaphore.tryAcquire(timeout, unit)` and runs the guarded operation unconditionally, so on a timeout the signal-processing critical section executes **without holding the permit** — defeating the mutual exclusion the semaphore exists to provide.
`debezium-connector-common/.../pipeline/signal/SignalProcessor.java` (around the `executeWithSemaphore` method, `SEMAPHORE_WAIT_TIME = 10`):
```java
private void executeWithSemaphore(Runnable operation) {
boolean acquired = false;
try {
acquired = semaphore.tryAcquire(SEMAPHORE_WAIT_TIME, TimeUnit.SECONDS);
operation.run(); // runs even when acquired == false
}
catch (InterruptedException e) {
LOGGER.error("Not able to acquire semaphore after {}s", SEMAPHORE_WAIT_TIME);
throw new DebeziumException("Not able to acquire semaphore during signaling processing", e);
}
finally {
if (acquired) {
semaphore.release();
}
}
}
```
`tryAcquire(timeout, unit)` returns `false` on timeout — it does not throw. The `catch (InterruptedException)` only fires if the waiting thread is itself interrupted, not when the permit is unavailable for 10s. So on timeout, execution falls through to `operation.run()` regardless. (The `finally` correctly skips `release()` since `acquired == false`, so there is no permit leak — but the exclusivity was already violated.)
### Why the guard matters
The single-permit `semaphore` serializes two entry points that run on **different threads**:
- `process()` — scheduled on `signalProcessorExecutor` (`scheduleAtFixedRate(this::process, ...)`).
- `processSourceSignal(partition)` — called inline from `EventDispatcher.dispatchDataChangeEvent()` on the streaming thread ("a synchronization point to immediately execute an eventual stop signal").
So the semaphore is exactly what keeps an external-channel signal action from running concurrently with a source-channel one.
### Failure scenario
1. A signal requests a long-running action — a blocking snapshot, or an `execute-snapshot` (INCREMENTAL) that loops `readChunk()` over a large table. `process()` holds the permit and runs it; this routinely exceeds 10s.
2. Meanwhile a signal-table row arrives; the streaming thread reaches `processSourceSignal()` → `executeWithSemaphore()`.
3. `tryAcquire(10, SECONDS)` waits 10s, returns `false`, no exception.
4. `operation.run()` executes anyway, concurrently with the still-running first action.
5. Both can mutate shared `@NotThreadSafe` state (e.g. `AbstractIncrementalSnapshotChangeEventSource.context` / its `window` `LinkedHashMap`), risking corrupted chunk bookkeeping, `ConcurrentModificationException`, or a stop-signal racing a snapshot start.
### Fix
Only enter the critical section when the permit was acquired. Two reasonable behaviours on timeout:
- **Fail-loud (matches the intent already encoded in the error message):** log and `throw new DebeziumException("Not able to acquire semaphore ...")`, i.e. make that existing message actually reachable for the timeout it was written for.
- **Fail-safe:** log a warning and skip this cycle (`return`); the periodic `process()` retries next interval and the source signal is re-read on the next dispatch.
I lean fail-safe (`return`) to avoid turning transient contention into a task failure, but I'm happy to follow the maintainers' preference. One related question: `SEMAPHORE_WAIT_TIME` is a hardcoded 10s, which makes a fast, non-flaky regression test awkward — would you be open to a small test seam (making the wait injectable/package-private) so the fix can be covered by a unit test?
I have a fix ready and can open a PR once the preferred behaviour is confirmed.
Contributor guide
Assessment
This issue has not been assessed yet.