Skip to content

Conversation

@mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Nov 1, 2022

This revert commit 2c8c288

Fixes #16907

Motivation

I think the problem described in #2993 is already solved by #2995, we don't need this workaround fix anymore.

Also, this workaround fix will introduce another problem and I will add a comment to the code.

Modifications

Verifying this change

  • Make sure that the change passes the CI checks.

  • doc-not-needed

@mattisonchao mattisonchao requested review from eolivelli and rdhabalia and removed request for rdhabalia November 1, 2022 09:44
try {
asyncReadEntry0(lh, position, callback, ctx);
} catch (Throwable t) {
log.warn("failed to read entries for {}-{}", lh.getId(), position, t);
Copy link
Member Author

@mattisonchao mattisonchao Nov 1, 2022

Choose a reason for hiding this comment

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

If callback.readEntryComplete(cachedEntry, ctx); throw any exception(line 226), the exception will also be catched by here, and invoke the callback.readEntryFailed(createManagedLedgerException(t), ctx);.

This means that in some cases we may call both methods, which is a risk to some recycled objects or other states.

Copy link
Member Author

Choose a reason for hiding this comment

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

And we shouldn't expire the cache for callback exceptions.

@github-actions
Copy link

github-actions bot commented Nov 1, 2022

@mattisonchao Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@mattisonchao mattisonchao added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Nov 1, 2022
@mattisonchao mattisonchao requested a review from Jason918 November 1, 2022 10:14
@mattisonchao mattisonchao reopened this Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Managed cursor asyncReplayEntries encounter NPE

1 participant