Skip to content

[fix][broker] Guard AsyncTokenBucket against long overflow#26

Open
codelipenghui wants to merge 2 commits intomasterfrom
penghui/fix-async-token-bucket-overflow
Open

[fix][broker] Guard AsyncTokenBucket against long overflow#26
codelipenghui wants to merge 2 commits intomasterfrom
penghui/fix-async-token-bucket-overflow

Conversation

@codelipenghui
Copy link
Owner

Main Issue: N/A

PIP: N/A

Motivation

The async dispatch rate limiter (AsyncTokenBucket) performs multiply-then-divide arithmetic using long. With large rates and longer rate periods (for example, 980,000,000 tokens per 10s), the multiplication can overflow and produce incorrect negative/intermediate values. In dispatch throttling this can keep available tokens at invalid values and effectively stall consumption.

Modifications

  • Added overflow-safe floor multiply/divide helper in AsyncTokenBucket:
    • Fast path for non-overflowing long multiplication.
    • Fallback to BigInteger for overflow cases.
    • Clamp large results to Long.MAX_VALUE.
  • Replaced overflow-prone arithmetic in token refill path:
    • calculateNewTokensSinceLastUpdate now uses safe multiply/divide.
    • Remainder-nanos back-calculation now uses safe multiply/divide as well.
  • Hardened throttling duration calculation:
    • calculateThrottlingDuration(long requiredTokens) now handles subtraction overflow and uses safe multiply/divide.
  • Added regression tests in AsyncTokenBucketTest:
    • shouldRefillTokensWithoutOverflowForLargeRateAnd10sPeriod
    • shouldCalculateThrottlingDurationWithoutOverflowForLargeNeedTokens

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Ran unit/regression tests:
    • ./mvnw -pl pulsar-broker -Dtest=AsyncTokenBucketTest -Dsurefire.failIfNoSpecifiedTests=false test
  • Ran dispatch-throttling integration tests:
    • ./mvnw -pl pulsar-broker -Dtest=SubscriptionMessageDispatchThrottlingTest -Dsurefire.failIfNoSpecifiedTests=false -DargLine='-Djdk.attach.allowAttachSelf=true -XX:+EnableDynamicAgentLoading' test

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

  • 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
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: N/A

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants