Skip to content

Fix a NPE bug after refactor recycler of BookieClient#4610

Merged
StevenLuMT merged 1 commit into
apache:masterfrom
TakaHiR07:fix_npe_after_refactor_recycler
Jun 6, 2025
Merged

Fix a NPE bug after refactor recycler of BookieClient#4610
StevenLuMT merged 1 commit into
apache:masterfrom
TakaHiR07:fix_npe_after_refactor_recycler

Conversation

@TakaHiR07

@TakaHiR07 TakaHiR07 commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

Motivation

After refactor recycler of bookieClient and test for a few days in cluster, there is a NPE bug.

企业微信截图_c8eea33b-7b52-49dc-8179-1f9f25fc35d1

In CompletionValue#timeout(), it would do errorOut() and then register latency in timeoutOpLogger. But errorOut() would trigger writeComplete() and then recycle the completionValue object. So the timeoutOpLogger is also recycle and throw NPE.

Changes

change the execute order: register latency first and then do errorOut().

Alternative fix is like:

void timeout() {
        OpStatsLogger logger = timeoutOpLogger;
        errorOut(BKException.Code.TimeoutException);
        logger.registerSuccessfulEvent(latency(),
                TimeUnit.NANOSECONDS);
}

But I think it is unnecessary to record the errorOut() time in prometheus, because this time cost has no influence of this timeout metric.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a Null Pointer Exception in CompletionValue.timeout by reordering the execution sequence.

  • The latency registration is now executed before calling errorOut.
  • The change prevents the timeoutOpLogger from being recycled before its usage.
Comments suppressed due to low confidence (1)

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/CompletionValue.java:92

  • The reordering of the latency registration before errorOut should resolve the NPE issue by ensuring that timeoutOpLogger is not recycled before its use. Please verify that this change does not introduce any side effects in the error handling logic.
errorOut(BKException.Code.TimeoutException);

@StevenLuMT StevenLuMT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good jobs

@StevenLuMT StevenLuMT merged commit 23289b7 into apache:master Jun 6, 2025
23 checks passed
@StevenLuMT

StevenLuMT commented Jun 12, 2025

Copy link
Copy Markdown
Member
  1. This is based on Fix Memory Leak In Netty Recycler of Bookie Client #4609, so I also added Fix Memory Leak In Netty Recycler of Bookie Client #4609 to release/4.17.2 , cc @zymap

  2. kept consistent without introducing new features Fix Memory Leak In Netty Recycler of Bookie Client #4609 to release/4.16.7

StevenLuMT pushed a commit that referenced this pull request Jun 12, 2025
Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 23289b7)
priyanshu-ctds pushed a commit to datastax/bookkeeper that referenced this pull request Jul 11, 2025
Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 23289b7)
(cherry picked from commit 9832318)
sandeep-ctds pushed a commit to datastax/bookkeeper that referenced this pull request Jul 22, 2025
Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 23289b7)
(cherry picked from commit 9832318)
@hangc0276 hangc0276 added this to the 4.18.0 milestone May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants