Skip to content

[ISSUE 11796] throw NPE when readEntry #11813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
casuallc wants to merge 3 commits into from
Closed

[ISSUE 11796] throw NPE when readEntry #11813

casuallc wants to merge 3 commits into from

Conversation

@casuallc
Copy link
Contributor

@casuallc casuallc commented Aug 27, 2021 •
edited by gaoran10
Loading

Fixes #11796

[ISSUE 11796] throw NPE when readEntry

No need doc.

gaoran10
gaoran10 reviewed Sep 1, 2021
View reviewed changes
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
OpReadEntry op = OpReadEntry.create(this, readPosition, numberOfEntriesToRead, callback,
ctx, maxPosition);
if (op.readPosition == null) {
return;
Copy link
Contributor

@gaoran10 gaoran10 Sep 1, 2021

Choose a reason for hiding this comment

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

Maybe we need to call the method callback.readEntriesFailed to make sure the caller gets a result.

Copy link
Contributor Author

@casuallc casuallc Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do not need call the method here, if OpReadEntry init failed, must ensure OpReadEntry->readEntriesFailed will be called, callback.readEntriesFailed will be called then.

315157973
315157973 reviewed Sep 2, 2021
View reviewed changes
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Outdated
PENDING_READ_OPS_UPDATER.incrementAndGet(this);
OpReadEntry op = OpReadEntry.create(this, readPosition, numOfEntriesToRead, callback, ctx, maxPosition);
ledger.asyncReadEntries(op);
if (op.readPosition != null) {
Copy link
Contributor

@315157973 315157973 Sep 2, 2021

Choose a reason for hiding this comment

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

op is not recycle if op.readPosition == null

By the way, could you add some unit test?

Copy link
Contributor Author

@casuallc casuallc Sep 2, 2021

Choose a reason for hiding this comment

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

op recycled when the method OpReadEntry->readEntriesFailed called.

At present, op init failed only if op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op); called.

I do not know how to test this case ...

@lhotari
Copy link
Member

lhotari commented Sep 8, 2021

good work @casuallc . The changes in this PR seem to be necessary for handling the failure case that is highlighted.

btw. There seems to be a similar change #11292 in progress, but that is taking a different approach for solving a similar issue. I'd prefer the approach used in this PR over the changes in 11292 since the solution in this PR is very simple.

@lhotari lhotari requested a review from merlimat September 8, 2021 07:05
@lhotari lhotari mentioned this pull request Sep 8, 2021
Closed
2 tasks
@lhotari lhotari requested a review from zymap September 9, 2021 06:30
@Jason918
Copy link
Contributor

Jason918 commented Sep 10, 2021

good work @casuallc . The changes in this PR seem to be necessary for handling the failure case that is highlighted.

btw. There seems to be a similar change #11292 in progress, but that is taking a different approach for solving a similar issue. I'd prefer the approach used in this PR over the changes in 11292 since the solution in this PR is very simple.

@lhotari
IMHO, there is race condition in this solution.
If op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op); is failed in OpReadEntry.create, this OpReadEntry is recycled in readEntriesFailed.
So if we check op.readPosition after OpReadEntry.create, this OpReadEntry object could be reused in other threads, and maybe the readPosition is properly initialized.

That's why I try to init readPosition after each OpReadEntry.create method in #11292 , causing the complexity.

@casuallc
Copy link
Contributor Author

casuallc commented Sep 10, 2021

good work @casuallc . The changes in this PR seem to be necessary for handling the failure case that is highlighted.
btw. There seems to be a similar change #11292 in progress, but that is taking a different approach for solving a similar issue. I'd prefer the approach used in this PR over the changes in 11292 since the solution in this PR is very simple.

@lhotari
IMHO, there is race condition in this solution.
If op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op); is failed in OpReadEntry.create, this OpReadEntry is recycled in readEntriesFailed.
So if we check op.readPosition after OpReadEntry.create, this OpReadEntry object could be reused in other threads, and maybe the readPosition is properly initialized.

That's why I try to init readPosition after each OpReadEntry.create method in #11292 , causing the complexity.

@Jason918 Yes, you are right. I should check op but not op.readPosition.
I changed the code, PTAL.

@lhotari lhotari mentioned this pull request Sep 23, 2021
Closed
@hezhangjian
Copy link
Member

hezhangjian commented Sep 23, 2021

@lhotari code managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java the callback paas the ctx as null.
But in class PersistentDispatcherSingleActiveConsumer readEntryFailed method acquire the ctx(Consumer) not be null.
So the code should be

opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("The ceilingKey(K key) method is used to return the " + "least key greater than or equal to the given key, or null if there is no such key"), ctx);

Am i right ?

@lhotari
Copy link
Member

lhotari commented Oct 15, 2021

Closing and re-opening this PR to trigger a fresh build.

@lhotari lhotari closed this Oct 15, 2021
@lhotari
Copy link
Member

lhotari commented Oct 15, 2021 •
edited
Loading

@casuallc It looks like your branch for this PR is gone and this PR cannot be re-opened. I closed it with the intention to trigger a rebuild. Rebuilding on CI was disabled and now I can see that the reason was that the original PR branch has been deleted.

@casuallc Would you mind creating a new PR with the same changes?

@lhotari
Copy link
Member

lhotari commented Oct 15, 2021

@nicoloboschi @eolivelli The changes in this PR would be useful for fixing a common NPE issue #11796.

@casuallc
Copy link
Contributor Author

casuallc commented Oct 18, 2021

@casuallc It looks like your branch for this PR is gone and this PR cannot be re-opened. I closed it with the intention to trigger a rebuild. Rebuilding on CI was disabled and now I can see that the reason was that the original PR branch has been deleted.

@casuallc Would you mind creating a new PR with the same changes?

#12396

@casuallc casuallc mentioned this pull request Nov 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@315157973 315157973 315157973 left review comments

@gaoran10 gaoran10 gaoran10 left review comments

@lhotari lhotari lhotari approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@eolivelli eolivelli Awaiting requested review from eolivelli

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@merlimat merlimat Awaiting requested review from merlimat

@zymap zymap Awaiting requested review from zymap

Assignees

@casuallc casuallc

Labels

area/broker

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

throw NPE when readEntry

6 participants

@casuallc @lhotari @Jason918 @hezhangjian @315157973 @gaoran10