Skip to content
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.

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

[fix][client] Fix NPE of MultiTopicsConsumerImpl due to race condition #18287

Conversation

codelipenghui
Copy link
Contributor

Motivation

 "exception":"java.util.concurrent.CompletionException: java.lang.NullPointerException\n\tat java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:314)\n\tat 
java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:319)\n\tat 
java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:718)\n\tat 
java.base/java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:478)\n\tat 
java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)\n\tat 
java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)\n\tat 
java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.jav
a:304)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat 
java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat 
org.apache.pulsar.shade.io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)\n\tat 
java.base/java.lang.Thread.run(Thread.java:829)\nCaused by: java.lang.NullPointerException: null\n\tat 
org.apache.pulsar.client.impl.MultiTopicsConsumerImpl.messageReceived(MultiTopicsConsumerImpl.java:298)\n\tat 
org.apache.pulsar.client.impl.MultiTopicsConsumerImpl.lambda$receiveMessageFromConsumer$7(MultiTopicsConsumerI
mpl.java:254)\n\tat java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:714)\n\t... 
8 common frames omitted\n" 

The stack trace is based on 2.9.2

The close and unsubscribe operation will change the unAckedMessageTracker to null which will cause the above problem.

Modifications

We should stop processing messages after the consumer is closed.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

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

@codelipenghui codelipenghui self-assigned this Nov 1, 2022
@codelipenghui codelipenghui added this to the 2.12.0 milestone Nov 1, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 1, 2022
@codelipenghui
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #18287 (30dd5c7) into master (0866c3a) will decrease coverage by 0.80%.
The diff coverage is 64.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18287      +/-   ##
============================================
- Coverage     38.97%   38.16%   -0.81%     
+ Complexity     8311     8014     -297     
============================================
  Files           683      659      -24     
  Lines         67325    65091    -2234     
  Branches       7217     6972     -245     
============================================
- Hits          26239    24845    -1394     
+ Misses        38079    37410     -669     
+ Partials       3007     2836     -171     
Flag Coverage Δ
unittests 38.16% <64.28%> (-0.81%) ⬇️

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

Impacted Files Coverage Δ
.../main/java/org/apache/pulsar/PulsarStandalone.java 0.00% <ø> (ø)
...pulsar/broker/service/PulsarCommandSenderImpl.java 69.63% <ø> (-4.73%) ⬇️
...ransaction/buffer/impl/InMemTransactionBuffer.java 0.00% <ø> (ø)
...nsaction/buffer/impl/TransactionBufferDisable.java 52.63% <ø> (ø)
.../metadata/v2/TransactionBufferSnapshotIndexes.java 0.00% <0.00%> (ø)
...a/v2/TransactionBufferSnapshotIndexesMetadata.java 0.00% <0.00%> (ø)
...oker/transaction/buffer/metadata/v2/TxnIDData.java 0.00% <0.00%> (ø)
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 0.00% <0.00%> (ø)
...ulsar/client/impl/transaction/TransactionImpl.java 0.00% <0.00%> (ø)
...rg/apache/pulsar/client/util/ExecutorProvider.java 0.00% <0.00%> (ø)
... and 109 more

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.

Good Catch!!!

@codelipenghui codelipenghui merged commit 516db51 into apache:master Nov 2, 2022
@codelipenghui codelipenghui deleted the penghui/fix-race-multiple-consumer branch November 2, 2022 10:19
@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Nov 2, 2022
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 3, 2022
codelipenghui added a commit that referenced this pull request Nov 3, 2022
codelipenghui added a commit that referenced this pull request Nov 3, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Nov 9, 2022
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.

None yet

5 participants