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.

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

DLQProducer's creation condition check is incorrect #9162

Closed
MarvinCai opened this issue Jan 10, 2021 · 0 comments · Fixed by #9166
Closed

DLQProducer's creation condition check is incorrect #9162

MarvinCai opened this issue Jan 10, 2021 · 0 comments · Fixed by #9166
Labels
type/bug The PR fixed a bug or issue reported a bug

Comments

@MarvinCai
Copy link
Contributor

When we check if we need to create producer for DLQ, we used double-checked locking, https://github.com/apache/pulsar/blame/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L701-L712
however we should do the second check after we acquired the lock, and the field we're checking should be a volatile field.
@MarvinCai MarvinCai added the type/bug The PR fixed a bug or issue reported a bug label Jan 10, 2021
@sijie sijie mentioned this issue Jan 10, 2021
Closed
@MarvinCai MarvinCai mentioned this issue Jan 10, 2021
Merged
1 task
sijie pushed a commit that referenced this issue Jan 18, 2021
@MarvinCai
Fix locking for ConsumerImpl when creating deadLetterProducer. (#9166)
b019c4b 
Fixes #9162

### Motivation
When we check if we need to create producer for DLQ, we used double-checked locking, https://github.com/apache/pulsar/blame/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L701-L712
however we should do the second check after we acquired the lock, and the field we're checking should be a volatile field.

### Modifications
Fix the double-checked locking and make the field volatile.
codelipenghui pushed a commit that referenced this issue Jan 19, 2021
@MarvinCai @codelipenghui
Fix locking for ConsumerImpl when creating deadLetterProducer. (#9166)
109ff54 
Fixes #9162

### Motivation
When we check if we need to create producer for DLQ, we used double-checked locking, https://github.com/apache/pulsar/blame/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L701-L712
however we should do the second check after we acquired the lock, and the field we're checking should be a volatile field.

### Modifications
Fix the double-checked locking and make the field volatile.

(cherry picked from commit b019c4b)
merlimat pushed a commit to merlimat/pulsar that referenced this issue Apr 6, 2021
@MarvinCai @merlimat
Fix locking for ConsumerImpl when creating deadLetterProducer. (apach…
d1caf6a 
…e#9166)

Fixes apache#9162

### Motivation
When we check if we need to create producer for DLQ, we used double-checked locking, https://github.com/apache/pulsar/blame/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L701-L712
however we should do the second check after we acquired the lock, and the field we're checking should be a volatile field.

### Modifications
Fix the double-checked locking and make the field volatile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix locking for ConsumerImpl when creating deadLetterProducer. MarvinCai/pulsar
1 participant
@MarvinCai