bpo-45416: Fix use of asyncio.Condition() with explicit Lock objects#28850
bpo-45416: Fix use of asyncio.Condition() with explicit Lock objects#28850serhiy-storchaka merged 8 commits intopython:mainfrom
Conversation
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I worked on this issue yesterday and I got similar code in general. I did not finish my work because I tried to only use the public API, and it was cumbersome.
Lib/test/test_asyncio/test_locks.py
Outdated
| async with cond: | ||
| pass |
There was a problem hiding this comment.
The exception is raised in await cond.acquire(). I think it would be better to call it directly.
| async with cond: | |
| pass | |
| await cond.acquire() |
It is raised because cond.acquire is the same as lock.acquire, and the latter fails because its loop is different.
But it would be nice to test less trivial case: cond.wait() which should raise RuntimeError in different code.
Lib/test/test_asyncio/test_locks.py
Outdated
| lock = asyncio.Lock() | ||
| loop = asyncio.new_event_loop() | ||
| self.addCleanup(loop.close) | ||
| lock._loop = loop |
There was a problem hiding this comment.
It would be nice to set it using only public API.
There was a problem hiding this comment.
I agree with it, but it says:
TypeError: As of 3.10, the *loop* parameter was removed from Lock() since it is no longer necessary
So I've added an explicit test for the TypeError and resorted back to lock._loop = loop.
There was a problem hiding this comment.
I meant using the public API which implicitly sets _loop. But it would be not easy. We can leave it to following PRs.
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
Thanks @achimnol for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
…ythonGH-28850) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit 1a78924) Co-authored-by: Joongi Kim <joongi@lablup.com>
|
GH-28856 is a backport of this pull request to the 3.10 branch. |
Both
asyncio.Lockandasyncio.Conditioncontacts the event loop in a deferred manner,when they really need to make a future to wait on.
So the loop-match validation should be delegated to
asyncio.mixins._LoopBoundMixin._get_loop()and removed from the constructor of
asyncio.Condition.The above change requires updates on two test cases:
test_explicit_loop()should not assert on the removed check in the constructor ofasyncio.Condition, and it should be updated to simply test setting of the explicit lockinstance. I also changed it to repeat the routines of
test_context_manager()to exposethe symptom reported in bpo-45416 when the change of
asyncio.Conditionconstructoris not applied.
test_ambiguous_loops()must be updated accordingly to trigger actualwaiting on the lock to happen and
_LoopBoundMixin._get_loop()to detectthe wrong event loop attachment.
https://bugs.python.org/issue45416