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

PubSub: correctly decrement the byte counter in drop() #4516

Closed
aongko opened this issue Dec 3, 2017 · 1 comment
Closed

PubSub: correctly decrement the byte counter in drop() #4516

aongko opened this issue Dec 3, 2017 · 1 comment
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@aongko
Copy link

aongko commented Dec 3, 2017

I'm looking at this 2 files:

Should the code in base.py line 215 be max instead of min?

self._bytes = min([self._bytes, 0])

Current code:

  1. current state self._bytes = 50
  2. then we call self._bytes -= 20
  3. now self._bytes == 30
  4. but then we do self._bytes = min([30, 0]) -> self._bytes = 0

Should be:

  1. current state self._bytes = 50
  2. then we call self._bytes -= 20
  3. now self._bytes == 30
  4. but then we do self._bytes = max([30, 0]) -> self._bytes = 30

even if we drop it below 0, we should use max:

  1. self._bytes = 10
  2. self._bytes -= 20
  3. self._bytes == -10
  4. min([-10, 0]) == -10 should be max([-10, 0]) == 0

Or is there something I miss?

@aongko
Copy link
Author

aongko commented Dec 3, 2017

Then I found #4514.
Hope it gets merged and released soon

@dhermes dhermes added api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 4, 2017
dhermes added a commit that referenced this issue Dec 4, 2017
… is non-negative (#4514)

The original intent was to keep `bytes` at or above `0`
but `min([-n, 0]) == -n` and more importantly `min([n, 0]) = 0`
so that `bytes` would be artificially dropped to `0` when
`drop()` is called.

Fixes #4516.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

No branches or pull requests

3 participants