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

Bugfix: Using max where min was used by mistake to ensure a value is non-negative #4514

Merged
merged 2 commits into from
Dec 4, 2017

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 2, 2017

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.

FWIW I was worried this was a data race issue, but I'm fairly certain it isn't. If in fact it turns out to be, we can add a no-op lock class on base.BasePolicy

import contextlib

@contextlib.contextmanager
def _no_op_lock():
    """No-op context manager."""
    yield

class BasePolicy(object):
    _LOCK_TYPE = _no_op_lock
    def __init__(self, ...):
        ...
        self._bytes_lock = self._LOCK_TYPE()

and then define _LOCK_TYPE = threading.Lock on our concrete implementation and use self._bytes_lock whenever modifying Policy().bytes.

Fixes #4516.

@dhermes dhermes added api: pubsub Issues related to the Pub/Sub API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Dec 2, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 2, 2017
@dhermes dhermes changed the title Swapping min for max with a logging. Bugfix: Using max where min was used by mistake to ensure a value is non-negative Dec 2, 2017
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.
@dhermes dhermes merged commit 7bb1321 into googleapis:master Dec 4, 2017
@dhermes dhermes deleted the bytes-use-max branch December 4, 2017 17:59
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. cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants