Refactor notifiers

[x42005e1f]

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.

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #725 will not alter performance

_{Comparing x42005e1f:master (405b1b3) with master (8ab8405)}

Summary

✅ 4 untouched benchmarks

[codecov[bot]]

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 :warning:

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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[asvetlov]

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]

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]

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]

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).