Skip to content

[Issue 8293][managed-ledger] Fix race condition in updating readPosition in ManagedCursorImpl - #8299

Merged
merlimat merged 1 commit into
apache:masterfrom
lhotari:lh-fix-race-in-ManagedCursorImpl
Oct 19, 2020
Merged

[Issue 8293][managed-ledger] Fix race condition in updating readPosition in ManagedCursorImpl#8299
merlimat merged 1 commit into
apache:masterfrom
lhotari:lh-fix-race-in-ManagedCursorImpl

Conversation

@lhotari

@lhotari lhotari commented Oct 19, 2020

Copy link
Copy Markdown
Member

Fixes #8293

Motivation

Regarding the race condition in #8229, it seems that ManagedCursorImpl.readPosition could get out of sync from OpReadEntry.readPosition if ManagedCursorImpl.readPosition gets updated after the OpReadEntry has been created since OpReadEntry's readPosition gets initialized from ManagedCursorImpl.readPosition.

The race condition seems to happen in this code in the setAcknowledgePosition method:

if (readPosition.compareTo(newMarkDeletePosition) <= 0) {
// If the position that is mark-deleted is past the read position, it
// means that the client has skipped some entries. We need to move
// read position forward
PositionImpl oldReadPosition = readPosition;
readPosition = ledger.getNextValidPosition(newMarkDeletePosition);
if (log.isDebugEnabled()) {
log.debug("[{}] Moved read position from: {} to: {}, and new mark-delete position {}", ledger.getName(),
oldReadPosition, readPosition, markDeletePosition);
}
}

The intent of this change is to properly handle concurrent updates when updating readPosition in setAcknowledgePosition method.

Modifications

The update of readPosition uses READ_POSITION_UPDATER.updateAndGet to handle concurrent updates to readPosition in setAcknowledgePosition method.

Verifying this change

There are currently no separate tests to verify this change.

@lhotari

lhotari commented Oct 19, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

1 similar comment
@lhotari

lhotari commented Oct 19, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label Oct 19, 2020
@merlimat merlimat added this to the 2.7.0 milestone Oct 19, 2020

@merlimat merlimat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Nice catch!

@merlimat
merlimat merged commit 6f4a4c3 into apache:master Oct 19, 2020
@lhotari
lhotari deleted the lh-fix-race-in-ManagedCursorImpl branch October 19, 2020 19:23
wolfstudy pushed a commit that referenced this pull request Oct 30, 2020
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Nov 13, 2020
flowchartsman pushed a commit to flowchartsman/pulsar that referenced this pull request Nov 17, 2020
merlimat pushed a commit to merlimat/pulsar that referenced this pull request Dec 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release/2.6.2 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in updating ManagedCursorImpl.readPosition

3 participants