Skip to content

Prevent double flush due to race in SingleDirectoryDbLedgerStorage#4305

Merged
dlg99 merged 1 commit into
apache:masterfrom
michaeljmarshall:show-potential-race-condition
May 31, 2024
Merged

Prevent double flush due to race in SingleDirectoryDbLedgerStorage#4305
dlg99 merged 1 commit into
apache:masterfrom
michaeljmarshall:show-potential-race-condition

Conversation

@michaeljmarshall

Copy link
Copy Markdown
Member

Disclaimer: I am not very familiar with this code, so please review carefully.

Motivation

We use the isFlushOngoing and the hasFlushBeenTriggered atomic booleans to prevent concurrent flushes. See:

// Write cache is full, we need to trigger a flush so that it gets rotated
// If the flush has already been triggered or flush has already switched the
// cache, we don't need to trigger another flush
if (!isFlushOngoing.get() && hasFlushBeenTriggered.compareAndSet(false, true)) {
// Trigger an early flush in background
log.info("Write cache is full, triggering flush");
executor.execute(() -> {
long startTime = System.nanoTime();
try {
flush();
} catch (IOException e) {
log.error("Error during flush", e);
} finally {
flushExecutorTime.addLatency(MathUtils.elapsedNanos(startTime), TimeUnit.NANOSECONDS);
}
});
}

By my reading of the code, it seems there is a brief period of time where the calling code will think it is valid to flush, even though there is a flush in progress.

Changes

In the SingleDirectoryDbLedgerStorage#swapWriteCache method modified by this PR, I propose a change to set isFlushOngoing to true before setting hasFlushBeenTriggered to false.

Master Issue: none

Note: I have not observed this race in an application,
but only noticed it when reading through the code.

@hezhangjian hezhangjian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dlg99

dlg99 commented May 29, 2024

Copy link
Copy Markdown
Contributor

closing to re-trigger CI

@dlg99 dlg99 closed this May 29, 2024
@dlg99 dlg99 reopened this May 29, 2024
@dlg99
dlg99 merged commit 3c5123d into apache:master May 31, 2024
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
@hangc0276 hangc0276 added this to the 4.18.0 milestone May 29, 2026
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.

4 participants