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

Refactor notifiers #725

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Refactor notifiers #725

wants to merge 9 commits into from

Conversation

x42005e1f
Copy link
Contributor

@x42005e1f x42005e1f commented Dec 22, 2024

What do these changes do?

This PR encapsulates the notification logic into new Condition subclasses, making it easier to understand the code and add new changes.

Are there changes in behavior for the user?

There are no behavior changes for users.

Copy link

codspeed-hq bot commented Dec 22, 2024

CodSpeed Performance Report

Merging #725 will not alter performance

Comparing x42005e1f:master (405b1b3) with master (8ab8405)

Summary

✅ 4 untouched benchmarks

Copy link

codecov bot commented Dec 22, 2024

Codecov Report

Attention: Patch coverage is 98.21429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.33%. Comparing base (8ab8405) to head (405b1b3).

Files with missing lines Patch % Lines
tests/test_mixed.py 75.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #725      +/-   ##
==========================================
- Coverage   95.40%   95.33%   -0.08%     
==========================================
  Files           5        5              
  Lines        1721     1694      -27     
  Branches      154      135      -19     
==========================================
- Hits         1642     1615      -27     
  Misses         52       52              
  Partials       27       27              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asvetlov
Copy link
Member

I have a biased opinion for the proposed change.
The code looks cleaner, sure.
But I don't like inheriting from lock classes. Could we use embedding instead of inheriting?

Also, please make a separate PR for fixing mentioned loop binding bugs.
Sorry, I've missed what the bug is. Many changes with parent._get_loop() look irrelevant, I cannot figure out what lines are related to the important part.

@x42005e1f
Copy link
Contributor Author

x42005e1f commented Dec 24, 2024

I used inheritance for the reason that it looks the most sensible here: existing Condition classes are extended with additional notification methods. The helper ones are prefixed with __, which avoids name conflicts, and conflicts with other names are extremely unlikely. Implementation via delegation is possible, but it is redundant and adds an extra frame to the call stack, which is insignificant, but still affects performance: calls are not cheap in Python.

The event loop binding bug is that if a user tries to call asynchronous methods from two different threads, then due to non-exclusive access to conditions there may be a situation when some asyncio.Condition will be bound to one event loop, but janus.Queue to a completely different one. As a result, the queue will be partially broken: some methods will work, some will not.

The fix is to prematurely bind the event loop in each asynchronous method, to safely prevent situations where asynchronous methods from two event loops are used. Of course, the README explicitly states not to support different event loops, but users can make mistakes, which is especially common in the asynchronous world. I did not put this fix in a separate PR for the reason that it is more a consequence of the notifier refactoring itself and can be seen as a side effect.

The main purpose of this PR is to secure janus from any incorrect changes that could break notification thread-safety. This is why the counter handling is encapsulated directly in the wait() call.

OK, I will update the PR soon according to your requests.

@x42005e1f
Copy link
Contributor Author

Since 1d91757, the fix is really a side effect: there is no way to set an event loop in the wait() method other than the one bound in async_lock, since it is called after the implicit async_lock.acquire(). I do not see a logical way to separate the fix from this PR.

@x42005e1f
Copy link
Contributor Author

Unless we can reverse the order of changes and apply the PR with the fix first. It can be seen as removing unnecessary parent._get_loop() in nowait methods (which became unnecessary since 57ac3fc).

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