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][broker]Sync topicPublishRateLimiter update #15599

Merged
merged 1 commit into from
May 28, 2022

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented May 14, 2022

Motivation

Synchronized update topicPublishRateLimiter in case of race condtion.
There is a race condtion when two threads update topicPublishRateLimiter concurrency:

  • topicPublishRateLimiter is null at the beginning
  • Thread1 update it with both publishThrottlingRateInByte < 0 and publishThrottlingRateInMsg < 0
  • Then thread1 stop at
    this.topicPublishRateLimiter = PublishRateLimiter.DISABLED_RATE_LIMITER;
  • Thread2 update it with publishThrottlingRateInByte > 0 or publishThrottlingRateInMsg > 0
  • And thread2 will set topicPublishRateLimiter a new limiter.
  • Then Thread1 resume, and set it to PublishRateLimiter.DISABLED_RATE_LIMITER. Then the new limiter setted by thread2 will never be accessed, which will produce resource leakages

Modifications

Synchronized method updatePublishDispatcher

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 14, 2022
@AnonHxy AnonHxy force-pushed the sync_topicPublishRateLimter branch from 98f2d17 to 9458fe0 Compare May 14, 2022 06:36
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 14, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy force-pushed the sync_topicPublishRateLimter branch from 9458fe0 to cdaac52 Compare May 14, 2022 14:07
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 15, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy force-pushed the sync_topicPublishRateLimter branch 2 times, most recently from 4ddbc98 to 2726281 Compare May 16, 2022 02:58
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 16, 2022

/pulsarbot run-failure-checks

2 similar comments
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 16, 2022

/pulsarbot run-failure-checks

@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 16, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy force-pushed the sync_topicPublishRateLimter branch from 2726281 to 0d3c41e Compare May 16, 2022 18:03
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 17, 2022

/pulsarbot run-failure-checks

@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 18, 2022

@Technoboy- @Jason918 @HQebupt PTAL

@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 22, 2022

/pulsarbot run-failure-checks

@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 23, 2022

@gaozhangmin @codelipenghui PTAL~

@Jason918 Jason918 added type/bug The PR fixed a bug or issue reported a bug release/2.10.1 labels May 28, 2022
@Jason918 Jason918 added this to the 2.11.0 milestone May 28, 2022
@Jason918 Jason918 merged commit 51e727f into apache:master May 28, 2022
codelipenghui pushed a commit to codelipenghui/incubator-pulsar that referenced this pull request Jun 7, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2022
(cherry picked from commit 51e727f)
(cherry picked from commit 1e02694)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants