Repository navigation
Close the event loop invoke_method creates for itself - #1215
magic-peach wants to merge 1 commit into
Conversation
Every synchronous invoke_method call went through get_running_loop failing with RuntimeError, then created a brand new event loop with asyncio.new_event_loop, ran it to completion, and left it open. Each call leaked a whole event loop, visible as an unclosed event loop ResourceWarning. Now the loop is closed in a finally block, but only when this call created it, not when it reused an already running one. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
JeffreyJPZ
left a comment
There was a problem hiding this comment.
Thanks @magic-peach as always, just have a few comments here
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter('always') | ||
| self.client.invoke_method(self.app_id, self.method_name, '') | ||
| self.client.invoke_method(self.app_id, self.method_name, '') | ||
|
|
||
| unclosed_loop_warnings = [w for w in caught if 'unclosed event loop' in str(w.message)] | ||
| self.assertEqual([], unclosed_loop_warnings) | ||
| self.assertTrue(asyncio.get_event_loop_policy().get_event_loop().is_closed()) |
There was a problem hiding this comment.
The warning messages are side effects, could we maybe assert on the loop directly by creating it and wrapping this in with patch("asyncio.new_event_loop", return_value=loop): or similar?
| self.assertEqual(b'STRING_BODY', response.data) | ||
| self.assertEqual(self.invoke_url, self.server.request_path()) | ||
|
|
||
| def test_invoke_closes_the_event_loop_it_creates(self): |
There was a problem hiding this comment.
Could we also have an equivalent test for the case where invoke fails?
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter('always') | ||
| self.client.invoke_method(self.app_id, self.method_name, '') | ||
| self.client.invoke_method(self.app_id, self.method_name, '') |
There was a problem hiding this comment.
Should we split these calls up so we know that each invoke closes its own loop? As it stands this test only checks the loop created by the last invoke (asyncio.get_event_loop_policy().get_event_loop() resolves to the event loop for the current thread, which is bound via asyncio.set_event_loop(loop) if I'm not mistaken).
CasperGN
left a comment
There was a problem hiding this comment.
Thanks. The leak is real, but closing the loop while it's still set as the thread's current loop trades it for a worse failure.
Blocking: the closed loop stays installed as the current event loop.
asyncio.set_event_loop(loop) (L164) runs before the call, and nothing unsets it after loop.close() (L172-L173). Any later code in the same thread that uses asyncio.get_event_loop() now gets a closed loop:
client.invoke_method('app', 'm', '')
asyncio.get_event_loop().run_until_complete(other())
# main: works
# this PR: RuntimeError: Event loop is closedThe new test asserts exactly this (get_event_loop_policy().get_event_loop().is_closed(), test L81), so it locks the regression in.
A fix that also stops the SDK from replacing a loop the caller had set: don't call set_event_loop for a loop you create.
loop = asyncio.new_event_loop()
try:
return loop.run_until_complete(awaitable)
finally:
loop.close()aiohttp finds the loop via get_running_loop() inside run_until_complete, so it doesn't need a current loop set. I'd avoid asyncio.run() here: it calls set_event_loop(None) on exit, which also clears a loop the caller set.
Non-blocking:
- The
owns_loop = Falsebranch (L159-L160) can't succeed. If a loop is already running in this thread,run_until_complete(L170) raises "This event loop is already running". That predates this PR, but with the fix above you can drop the branch, or raise a clear error that points callers toinvoke_method_async. asyncio.get_event_loop_policy()(test L81) is deprecated in 3.14. Once the test checks "no unclosed-loop warning, and a loop the caller set is still current and open", it won't need it.- The branch is behind
main.
Description
Every synchronous invoke_method call went through get_running_loop failing with RuntimeError, then created a brand new event loop with asyncio.new_event_loop, ran it to completion, and left it open. Each call leaked a whole event loop, visible as an unclosed event loop ResourceWarning. Now the loop is closed in a finally block, but only when this call created it, not when it reused an already running one.
Issue reference
N/A, self-discovered while reviewing the HTTP invocation client, no existing issue filed.
Checklist