Skip to content

[Broker] Fix possible data race in getFirstAvailableConsumerPermits - #10831

Merged
merlimat merged 2 commits into
apache:masterfrom
lhotari:lh-fix-possible-race-in-getFirstAvailableConsumerPermits
Jun 5, 2021
Merged

[Broker] Fix possible data race in getFirstAvailableConsumerPermits#10831
merlimat merged 2 commits into
apache:masterfrom
lhotari:lh-fix-possible-race-in-getFirstAvailableConsumerPermits

Conversation

@lhotari

@lhotari lhotari commented Jun 4, 2021

Copy link
Copy Markdown
Member

Motivation

  • there's a chance for a data race so that the consumer's available permits goes
    below zero after it has been checked in isConsumerAvailable

Modifications

  • check that availablePermits > 0 before returning

@lhotari
Fix possible race in getFirstAvailableConsumerPermits
c297074 
- there's a chance for a race so that the consumer's available permits goes
  below zero after it has been checked in "isConsumerAvailable"
@lhotari lhotari added the type/bug The PR fixed a bug or issue reported a bug label Jun 4, 2021
@lhotari lhotari added this to the 2.8.0 milestone Jun 4, 2021
@lhotari lhotari self-assigned this Jun 4, 2021
@michaeljmarshall

michaeljmarshall commented Jun 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

@lhotari - in order to call get on the atomic wrapper just once, what if we added a new method that returns an Optional<Integer> that is a combination of the isConsumerAvailable and the getAvailablePermits methods? It would introduce additional overhead for the new objects, so perhaps it's an unnecessary change. Do you have a preference?

codelipenghui
codelipenghui reviewed Jun 5, 2021
View reviewed changes
Comment thread .../java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java Outdated
Comment on lines 675 to 680
if (isConsumerAvailable(consumer)) {
return consumer.getAvailablePermits();
int availablePermits = consumer.getAvailablePermits();
if (availablePermits > 0) {
return availablePermits;
}
}

@codelipenghui codelipenghui Jun 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is better to change to:

if (consumer != null && !consumer.isBlocked()) {
       return consumer.getAvailablePermits();
}

So that we don't need to check availablePermits twice

@lhotari lhotari Jun 5, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@codelipenghui Done. PTAL

@codelipenghui codelipenghui modified the milestones: 2.8.0, 2.9.0 Jun 5, 2021
@lhotari

lhotari commented Jun 5, 2021

Copy link
Copy Markdown
Member Author

@lhotari - in order to call get on the atomic wrapper just once, what if we added a new method that returns an Optional<Integer> that is a combination of the isConsumerAvailable and the getAvailablePermits methods? It would introduce additional overhead for the new objects, so perhaps it's an unnecessary change. Do you have a preference?

In this part of code I believe that there's a goal of zero allocations since this is part of a hot code path. Therefore, omitting the usage of Optional is also desired.

michaeljmarshall
michaeljmarshall approved these changes Jun 5, 2021
View reviewed changes

@michaeljmarshall michaeljmarshall 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.

LGTM

@merlimat
merlimat merged commit bd568bc into apache:master Jun 5, 2021
@merlimat merlimat modified the milestones: 2.9.0, 2.8.0 Jun 5, 2021
eolivelli pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2021
@lhotari @eolivelli
[Broker] Fix possible data race in getFirstAvailableConsumerPermits (a…
9f30951 
…pache#10831)

* Fix possible race in getFirstAvailableConsumerPermits

- there's a chance for a race so that the consumer's available permits goes
  below zero after it has been checked in "isConsumerAvailable"

* Address review feedback

(cherry picked from commit bd568bc)
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
@lhotari @yangl
[Broker] Fix possible data race in getFirstAvailableConsumerPermits (a…
fbcfcf7 
…pache#10831)

* Fix possible race in getFirstAvailableConsumerPermits

- there's a chance for a race so that the consumer's available permits goes
  below zero after it has been checked in "isConsumerAvailable"

* Address review feedback
@codelipenghui codelipenghui added cherry-picked/branch-2.8 Archived: 2.8 is end of life and removed cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.1 release/2.7.3 labels Jun 25, 2021
@codelipenghui

codelipenghui commented Jun 27, 2021

Copy link
Copy Markdown
Contributor

This PR fixes the issues introduce by #10417, so don't need to cherry-pick to branch-2.7

bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@lhotari @splunk-xingw
[Broker] Fix possible data race in getFirstAvailableConsumerPermits (a…
45e997a 
…pache#10831)

* Fix possible race in getFirstAvailableConsumerPermits

- there's a chance for a race so that the consumer's available permits goes
  below zero after it has been checked in "isConsumerAvailable"

* Address review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merlimat merlimat merlimat approved these changes
@codelipenghui codelipenghui codelipenghui approved these changes
@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes
@rdhabalia rdhabalia Awaiting requested review from rdhabalia
@eolivelli eolivelli Awaiting requested review from eolivelli

Assignees

@lhotari lhotari

Labels

type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Milestone

2.8.0

Development

Successfully merging this pull request may close these issues.

4 participants

@lhotari @michaeljmarshall @codelipenghui @merlimat