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

Add backpressure mechanism to limit the number of concurrent outgoing calls #101

Merged
merged 6 commits into from
Jan 18, 2022

Conversation

bachya
Copy link
Contributor

@bachya bachya commented Jan 17, 2022

This PR applies a backpressure mechanism so that extremely chatty clients don't add congestion to the server. The mechanism is controlled with a max_concurrent_outgoing_calls parameter that can be applied to the client during instantiation:

  • A max_concurrent_outgoing_calls value of None means that no back pressure is applied
  • A max_concurrent_outgoing_calls value with a positive int indicates that no more than max_concurrent_outgoing_calls outgoing calls can happen at any given time.

Fixes #29

@frederikaalund Some comments for your review and consideration:

  • As discussed, the fact that max_concurrent_outgoing_calls can be None (and is so by default) should retain backward compatibility.
  • I didn't see any other spots in the README where specific functionality is shown via example; let me know if you think one is warranted here.
  • I didn't apply any linting/styling since I couldn't determine whether a standard was already used. I did endeavor to follow the existing standard for type hinting.
  • I didn't add any tests for this since the library as a whole doesn't seem to carry many; let me know if you would prefer I add some.

@bachya bachya changed the title Add back pressure mechanism Add backpressure mechanism to limit the number of concurrent outgoing calls Jan 17, 2022
Copy link
Collaborator

@frederikaalund frederikaalund left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. 👍 Please have a look at my comments in the review. Let me know if anything is unclear, or, if you have other suggestions. :)

asyncio_mqtt/client.py Outdated Show resolved Hide resolved
asyncio_mqtt/client.py Outdated Show resolved Hide resolved
asyncio_mqtt/client.py Outdated Show resolved Hide resolved
asyncio_mqtt/client.py Outdated Show resolved Hide resolved
asyncio_mqtt/client.py Outdated Show resolved Hide resolved
@frederikaalund
Copy link
Collaborator

I'd also like a comment somewhere along these lines:

# TODO: Simplify the logic that surrounds `self._outgoing_calls_sem` with `nullcontext` when we
# support Python 3.10 (`nullcontext` becomes async-aware in 3.10).
# See: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

@frederikaalund
Copy link
Collaborator

I added one last comment. When that is done this PR looks good to me. 👍 Thanks for the patience with my pedantic obsession with typing. 😅

@frederikaalund
Copy link
Collaborator

Looks good to me now. Thank you for your contribution to asyncio-mqtt. 👍

@frederikaalund frederikaalund merged commit 301a28a into empicano:master Jan 18, 2022
@bachya
Copy link
Contributor Author

bachya commented Jan 18, 2022

Thank you so much for your support! So I can plan: when are you thinking about cutting a new release?

@bachya bachya deleted the backpressure branch January 18, 2022 21:33
@frederikaalund
Copy link
Collaborator

First thing tomorrow. I'll keep the notification e-mail about this PR in my inbox to remind me.

@frederikaalund
Copy link
Collaborator

I just uploaded v0.12.0 to PyPI.

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.

pending publish calls
2 participants