Skip to content

[fix][broker] Fix NPE and annotate nullable return values for ManagedCursorContainer #24706

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
Merged
BewareMyPower merged 2 commits into from
Sep 5, 2025
Merged

[fix][broker] Fix NPE and annotate nullable return values for ManagedCursorContainer #24706

BewareMyPower merged 2 commits into from
Sep 5, 2025

Conversation

@BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Sep 5, 2025 •
edited
Loading

Motivation

I observed the following NPE when I ran some tests.

2025-09-05 10:49:38.826 [pulsar-msg-expiry-monitor-OrderedScheduler-0-0] WARN  org.apache.bookkeeper.common.util.SingleThreadSafeScheduledExecutorService - Unexpected throwable from task class com.google.common.util.concurrent.MoreExecutors$ScheduledListeningDecorator$NeverSuccessfulListenableFutureTask: Cannot invoke "org.apache.bookkeeper.mledger.impl.ManagedCursorContainer$CursorInfo.getCursor()" because the return value of "org.apache.bookkeeper.mledger.impl.ManagedCursorContainer.getCursorWithOldestPosition()" is null
java.lang.NullPointerException: Cannot invoke "org.apache.bookkeeper.mledger.impl.ManagedCursorContainer$CursorInfo.getCursor()" because the return value of "org.apache.bookkeeper.mledger.impl.ManagedCursorContainer.getCursorWithOldestPosition()" is null
	at org.apache.pulsar.broker.service.persistent.PersistentTopic.checkMessageExpiryWithSharedPosition(PersistentTopic.java:2169)

It's because getCursorWithOldestPosition could return null but checkMessageExpiryWithSharedPosition does not apply null validation.

Modifications

Add @Nullable annotations to all ManagedCursorContainer's methods of that could return null. Then check all non-test code and add null validation if missed:

  1. The checkMessageExpiryWithSharedPosition added in [improve][broker]Find the target position at most once, during expiring messages for a topic, even though there are many subscriptions #24622
  2. The missed null validation for getSlowedConsumer since a very early commit, but not sure why the NPE was never reported

P.S. Using Optional might be a better alternative to force users to check null because many developers still ignore warnings. To avoid unnecessary heap allocation and API changes, I didn't change the method signature.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@BewareMyPower BewareMyPower self-assigned this Sep 5, 2025
@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 5, 2025
poorbarcode
poorbarcode approved these changes Sep 5, 2025
View reviewed changes
Copy link
Contributor

@poorbarcode poorbarcode left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it

@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.20%. Comparing base (669ab61) to head (1a1e5e2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...che/pulsar/broker/service/BacklogQuotaManager.java 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24706      +/-   ##
============================================
+ Coverage     74.18%   74.20%   +0.01%     
+ Complexity    33472    33463       -9     
============================================
  Files          1895     1895              
  Lines        147968   147976       +8     
  Branches      17134    17137       +3     
============================================
+ Hits         109768   109803      +35     
+ Misses        29448    29411      -37     
- Partials       8752     8762      +10
Flag Coverage Δ
inttests 26.26% <0.00%> (-0.08%) ⬇️
systests 22.60% <0.00%> (-0.10%) ⬇️
unittests 73.72% <66.66%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...a/org/apache/bookkeeper/mledger/ManagedLedger.java 0.00% <ø> (ø)
...ookkeeper/mledger/impl/ManagedCursorContainer.java 87.50% <ø> (ø)
...sar/broker/service/persistent/PersistentTopic.java 79.39% <100.00%> (+0.10%) ⬆️
...che/pulsar/broker/service/BacklogQuotaManager.java 63.97% <0.00%> (-0.96%) ⬇️

... and 82 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@BewareMyPower BewareMyPower merged commit e6f66df into apache:master Sep 5, 2025
51 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/npe-expiry branch September 5, 2025 07:54
@Technoboy- Technoboy- added this to the 4.2.0 milestone Sep 7, 2025
lhotari pushed a commit that referenced this pull request Sep 8, 2025
@BewareMyPower @lhotari
[fix][broker] Fix NPE and annotate nullable return values for Managed…
77d18bf 
…CursorContainer (#24706)

(cherry picked from commit e6f66df)
Technoboy- added a commit that referenced this pull request Sep 10, 2025
@Technoboy-
[fix][broker] Fix NPE and annotate nullable return values for Managed…
7988f4f 
…CursorContainer (#24706)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 12, 2025
@Technoboy- @manas-ctds
[fix][broker] Fix NPE and annotate nullable return values for Managed…
c046381 
…CursorContainer (apache#24706)

(cherry picked from commit 7988f4f)
nborisov pushed a commit to nborisov/pulsar that referenced this pull request Sep 12, 2025
@Technoboy-
[fix][broker] Fix NPE and annotate nullable return values for Managed…
579e972 
…CursorContainer (apache#24706)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 12, 2025
@Technoboy- @srinath-ctds
[fix][broker] Fix NPE and annotate nullable return values for Managed…
1a368f7 
…CursorContainer (apache#24706)

(cherry picked from commit 7988f4f)
KannarFr pushed a commit to CleverCloud/pulsar that referenced this pull request Sep 22, 2025
@BewareMyPower @KannarFr
[fix][broker] Fix NPE and annotate nullable return values for Managed…
0d6d855 
…CursorContainer (apache#24706)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
@BewareMyPower @walkinggo
[fix][broker] Fix NPE and annotate nullable return values for Managed…
e8ffd42 
…CursorContainer (apache#24706)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@poorbarcode poorbarcode poorbarcode approved these changes

@lhotari lhotari lhotari approved these changes

@coderzc coderzc coderzc approved these changes

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

Assignees

@BewareMyPower BewareMyPower

Labels

cherry-picked/branch-4.0 cherry-picked/branch-4.1 doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/4.0.7 release/4.1.1

Projects

None yet

Milestone

4.2.0

Development

Successfully merging this pull request may close these issues.

6 participants

@BewareMyPower @codecov-commenter @lhotari @poorbarcode @coderzc @Technoboy-