[Issue-11282] Fix NPE in OpReadEntry

[Jason918]

Fixes #11282

Motivation

fix NPE in OpReadEntry, see issue #11282 .

Modifications

remove opReadEntry.readEntriesFailed in ManagedLedgerImpl::startReadOperationOnLedger, the state checking is handled by all the callers in OpReadEntry.

Verifying this change

• Make sure that the change passes the CI checks.

Added new test case testManagedLedgerWithReadEntryWithoutNewerEntry.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

• Dependencies (does it add or upgrade a dependency): (no)
• The public API: (no)
• The schema: (no)
• The default values of configurations: (no)
• The wire protocol: (no)
• The rest endpoints: (no)
• The admin cli options: (no)
• Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

• no-need-doc

bug fix.

[littleorca]

I think this won't work. When readEntriesFailed is called during initiate readPosition, the property readPosition was not assigned yet, and readEntriesFailed has a chance to call readPosition.getLedgerId()，which will cause another NPE.

Further more, I guess the callbacks should be concerned also.

[Jason918]

I think this won't work. When readEntriesFailed is called during initiate readPosition, the property readPosition was not assigned yet, and readEntriesFailed has a chance to call readPosition.getLedgerId()，which will cause another NPE.

Further more, I guess the callbacks should be concerned also.

It's NoMoreEntriesToReadException in startReadOperationOnLedger right? So it's not running into the if branch for readPosition.getLedgerId().

And the callback is also assigned before the assignment of readPosition.

[Jason918]

@littleorca After deep looking into this case, I found that more serious issue is that, when ManagedLedgerException.NoMoreEntriesToReadException occurs in OpReadEntry.create, this OpReadEntry object is actually recycled when create method finished. Any usage of this object would lead to some unexpected behavior.

[littleorca]

I think this won't work. When readEntriesFailed is called during initiate readPosition, the property readPosition was not assigned yet, and readEntriesFailed has a chance to call readPosition.getLedgerId()，which will cause another NPE.
Further more, I guess the callbacks should be concerned also.

It's NoMoreEntriesToReadException in startReadOperationOnLedger right? So it's not running into the if branch for readPosition.getLedgerId().

And the callback is also assigned before the assignment of readPosition.

Seems you are right. I've tried to reorder statements in create() method and ended up in failure, I probably messed up some details about the failure.

[lhotari]

FYI, there are some thread safety issues in OpReadEntry . PR #11387 attempts to fix some of the thread safety issues.

[hangc0276]

move to 2.8.2.

[lhotari]

There seems to be an alternative approach to solve the NPE in PR #11813 . I currently prefer that change since it's fairly simple.

[Jason918]

There seems to be an alternative approach to solve the NPE in PR #11813 . I currently prefer that change since it's fairly simple.

There maybe some race conditions in PR #11813, explained the details in that PR, PTAL. :)

[315157973]

@lhotari @codelipenghui @hangc0276 PTAL

[github-actions]

@Jason918:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

[Anonymitaet]

@Jason918 seems this is a bug fix (no need to update docs)?

[Jason918]

@Jason918 seems this is a bug fix (no need to update docs)?

Yes.

[github-actions]

@Jason918:Thanks for providing doc info!

[Jason918]

Close by duplication.

[michaeljmarshall]

Removing release label and milestone since the PR is closed. Please re-add if we need to open the PR.