Improve block proposal cancellation to (best effort) avoid concurrency issues#10219
Improve block proposal cancellation to (best effort) avoid concurrency issues#10219fab-10 wants to merge 2 commits intobesu-eth:mainfrom
Conversation
…ection When block creation is cancelled or times out, the selection thread may still be running briefly. This change adds a CountDownLatch to internal tx selection (mirroring the existing plugin selection mechanism) and extracts a shared waitForCancellationToBeProcessed method that correctly handles negative remaining-time values and logs the outcome of the wait. Exception handling in both selection phases is split by type so that rollback() is only called for ExecutionException, where the selection thread is guaranteed to have finished. CancellationException and InterruptedException no longer trigger a rollback, removing a potential race on shared world state. In MergeCoordinator, exceptions thrown after a cancellation are now logged at INFO with guidance to report if unexpected, rather than at WARN, reducing noise from the expected concurrency edge cases during block proposal cancellation. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
changed the title
- Add test verifying CancellationException during plugin selection is handled gracefully (no exception propagated to caller) - Add test verifying internal selection CountDownLatch causes buildTransactionListForBlock() to wait for the selection thread - Add test verifying Throwable thrown after block creation cancellation is handled gracefully (logged at INFO, not propagated) - Remove early return from timeLimitedSelection when isCancelled is true: the guard was causing validPendingTransactionIsNotIncludedIf SelectionCancelled to fail because evaluatePendingTransaction (which marks each tx as SELECTION_CANCELLED) was never reached; the check is unnecessary since evaluatePendingTransaction already handles isCancelled on every iteration without touching world state Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
force-pushed
the
fix-cancelled-block-creation-exception
branch
from
f391044 to
cf708bc
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves block proposal cancellation behavior to reduce race conditions between transaction selection threads and post-selection world state mutations, and reduces log noise for expected cancellation-related exceptions.
Changes:
- Add a
CountDownLatchto internal tx selection and a sharedwaitForCancellationToBeProcessed(...)helper to best-effort wait for selection threads to stop. - Refine exception handling to avoid rollback on cancellation/interruption paths and to handle plugin-selection cancellation gracefully.
- Demote post-cancellation block creation exceptions in
MergeCoordinatorfrom WARN to INFO and add tests covering these scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java | Adds latches + wait helper and refines cancellation/interrupt/exception handling during selection. |
| ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java | Adds tests for external cancellation during plugin selection and waiting for internal selection thread completion. |
| consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java | Logs post-cancellation exceptions at INFO with guidance; keeps WARN for non-cancelled failures. |
| consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java | Adds a regression test ensuring post-cancellation exceptions don’t escape the background task. |
| context, | ||
| nanosToMillis(maxWaitTimeNanos), | ||
| ex); | ||
| throw new RuntimeException(ex); |
There was a problem hiding this comment.
This converts InterruptedException into a RuntimeException without restoring the thread interrupt flag. If an interrupt happens here, best practice is to call Thread.currentThread().interrupt() and then return (best-effort) or rethrow InterruptedException so upper layers can handle cancellation correctly. Throwing a new runtime exception here can also defeat the goal of 'best effort' completion during cancellation paths.
| throw new RuntimeException(ex); | |
| Thread.currentThread().interrupt(); | |
| return; |
| } catch (InterruptedException e) { | ||
| LOG.debug( | ||
| "Transaction selection interrupted during execution, finalizing with current progress", | ||
| e); |
There was a problem hiding this comment.
Both cancellation and interruption are swallowed here. For InterruptedException, consider restoring the interrupt status (Thread.currentThread().interrupt()) so higher-level code can observe the interruption. Right now the interrupt is cleared by catching the exception, which can cause the rest of block creation to continue even though the thread was interrupted.
| e); | |
| e); | |
| Thread.currentThread().interrupt(); |
| CompletableFuture.runAsync( | ||
| () -> { | ||
| try { | ||
| pluginTaskStarted.await(); | ||
| selectorRef.get().cancel(); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| }); | ||
|
|
||
| // must complete without throwing; the old code propagated CancellationException out of | ||
| // pluginTimeLimitedSelection, which would bubble up through buildTransactionListForBlock() | ||
| final var results = selectorRef.get().buildTransactionListForBlock(); | ||
| assertThat(results).isNotNull(); |
There was a problem hiding this comment.
This async task is never awaited and pluginTaskStarted.await() is unbounded. If the latch is never counted down (e.g., due to a failure before plugin selection starts), this can leak a blocked task into the common pool and make the test suite flaky. Consider keeping the returned CompletableFuture, using a bounded await (timeout), and ensuring the task completes (e.g., join with timeout) before the test ends.
| CompletableFuture.runAsync( | |
| () -> { | |
| try { | |
| pluginTaskStarted.await(); | |
| selectorRef.get().cancel(); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| }); | |
| // must complete without throwing; the old code propagated CancellationException out of | |
| // pluginTimeLimitedSelection, which would bubble up through buildTransactionListForBlock() | |
| final var results = selectorRef.get().buildTransactionListForBlock(); | |
| assertThat(results).isNotNull(); | |
| final CompletableFuture<Void> cancellationTask = | |
| CompletableFuture.runAsync( | |
| () -> { | |
| try { | |
| if (pluginTaskStarted.await(5, java.util.concurrent.TimeUnit.SECONDS)) { | |
| selectorRef.get().cancel(); | |
| } | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| }); | |
| // must complete without throwing; the old code propagated CancellationException out of | |
| // pluginTimeLimitedSelection, which would bubble up through buildTransactionListForBlock() | |
| final var results = selectorRef.get().buildTransactionListForBlock(); | |
| assertThat(results).isNotNull(); | |
| cancellationTask.orTimeout(5, java.util.concurrent.TimeUnit.SECONDS).join(); |
| // On processing: cancel the selector (from within the selection thread), sleep to simulate | ||
| // work that continues after the interrupt, then mark as finished. | ||
| final Answer<TransactionProcessingResult> slowCancelAnswer = | ||
| invocation -> { | ||
| selectorRef.get().cancel(); // triggers FutureTask.cancel(true) on current task | ||
| try { | ||
| Thread.sleep(300); // wakes immediately via InterruptedException | ||
| } catch (InterruptedException e) { | ||
| try { | ||
| Thread.sleep(200); // simulate cleanup still running after interrupt | ||
| } catch (InterruptedException ie) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } | ||
| selectionThreadFinished.set(true); |
There was a problem hiding this comment.
This test relies on fixed Thread.sleep(...) timing to model post-interrupt work, which is prone to flakiness under load (different schedulers/CI environments). Prefer coordination primitives (e.g., latches/barriers) to deterministically block/unblock the selection thread and to signal 'cleanup finished' without depending on wall-clock sleeps.
| // concurrency issues, so inform the user how to interpret that possibility | ||
| LOG.info( | ||
| "Got an exception after cancellation of block creation for payload id {}. " | ||
| + "This is expected if previous log alerted about that, otherwise please report", |
There was a problem hiding this comment.
The INFO message is hard to interpret (“previous log alerted about that” is ambiguous and grammatically awkward). Consider making it actionable by referencing the actual condition the user should look for (e.g., timeout/cancellation log line name) and clearly stating when to report (e.g., 'If you do not see a prior cancellation/timeout log for this payload, please report this stack trace').
| + "This is expected if previous log alerted about that, otherwise please report", | |
| + "This is expected if you already saw the earlier " | |
| + "\"Block creation for payload id {} has been cancelled, reason {}\" log for " | |
| + "this payload. If you do not see that earlier cancellation log for this " | |
| + "payload, please report this stack trace.", |
Summary
Today I spent some time reviewing the exception reported below, and it ended up to be an interesting investigation, that led to an improvement of the cancellation path to avoid (as much as possible) this concurrency issue.
Usually, like in the reported case, the exception is not an issue, since there were previous proposals available to return, but in the case the cancellation happens on the very first block creation iteration, then it could result in an empty proposal returned, instead of something better.
When block creation is cancelled or times out, the selection thread may still be running briefly after
FutureTask.get()returns. Without proper synchronisation, post-selection steps (withdrawals, EL requests, rewards processing) can race against the still-running thread on the shared world state.Exactly what happened in the above exception, the race condition between the withdrawals processing and the cancelled but still running tx selection thread.
CountDownLatchto internal tx selection — mirrors the existing mechanism in plugin selection; thefinallyblock counts down after the selection loop completes, giving a reliable signal that the thread has stopped.waitForCancellationToBeProcessed— shared helper called after both plugin and internal selection phases when the latch is non-zero. Correctly handles a negative remaining-time value (logs and returns immediately instead of silently passing a negative timeout toCountDownLatch.await) and logs the outcome of the wait.rollback()is now only called forExecutionException, where the callable has already thrown and thefinallyblock is guaranteed to have run (latch at 0, selection thread stopped).CancellationExceptionandInterruptedExceptionno longer trigger a rollback, removing a potential race where the selection thread could still be mutatingselectionPendingActionsand the world state updaters.MergeCoordinator— exceptions thrown afterisBlockCreationCancelledis true are now logged at INFO with a message guiding the user to report if unexpected, rather than at WARN, reducing noise from the expected concurrency edge cases during block proposal cancellation.Test plan
BlockTransactionSelectorandMergeCoordinatorpassengine_forkchoiceUpdatedfollowed by a new payload)🤖 Generated with Claude Code