Fix batch ackset recycled multiple times. #7409
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codelipenghui
added
release/2.6.1
type/bug
merlimat
reviewed
Jul 1, 2020
|
|
||
| public void recycle() { | ||
| if (recyclerHandle != null) { | ||
| if (recyclerHandle != null && recycled.compareAndSet(false, true)) { |
Contributor
There was a problem hiding this comment.
This is still not safe. There cannot be a race condition to resolve the recycling.
In this case it can happen:
thread-1calls recycle and CAS succeedsthread-2gets a the object from poolthread-3seesrecycled==falseand does the CAS again, recycle a different object that is in use.
You cannot use Recyclable for objects with a shared ownership. Either change the logic to avoid race conditions, or use ref-counting.
Contributor
Author
There was a problem hiding this comment.
Make sense. Thanks.
Contributor
Author
There was a problem hiding this comment.
@merlimat I have addressed your comments, please take a look again, thanks.
Contributor
Author
|
/pulsarbot run-failure-checks |
codelipenghui
commented
Jul 3, 2020
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
Outdated
Show resolved
Hide resolved
…ections/BitSetRecyclable.java
Contributor
Author
|
/pulsarbot run-failure-checks |
merlimat
approved these changes
Jul 6, 2020
codelipenghui
added a commit
to streamnative/pulsar-archived
that referenced
this pull request
Jul 14, 2020
* Fix batch ackset recycled multiple times. * Apply comments. * Update pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java (cherry picked from commit 25a6907)
cdbartholomew
pushed a commit
to kafkaesque-io/pulsar
that referenced
this pull request
Jul 21, 2020
* Fix batch ackset recycled multiple times. * Apply comments. * Update pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
cdbartholomew
pushed a commit
to kafkaesque-io/pulsar
that referenced
this pull request
Jul 24, 2020
* Fix batch ackset recycled multiple times. * Apply comments. * Update pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
wolfstudy
pushed a commit
that referenced
this pull request
Jul 29, 2020
* Fix batch ackset recycled multiple times. * Apply comments. * Update pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java (cherry picked from commit 25a6907)
huangdx0726
pushed a commit
to huangdx0726/pulsar
that referenced
this pull request
Aug 24, 2020
* Fix batch ackset recycled multiple times. * Apply comments. * Update pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7384
Motivation
Fix batch ackset recycled multiple times. The root cause is a race condition in group ack flush and cumulative Ack. So add recycled state check for the ackset.
Verifying this change
New unit test added.
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation