fix: close loop from run_async #4734
Conversation
jina/helper.py
Outdated
| return get_or_reuse_loop().run_until_complete(func(*args, **kwargs)) | ||
| loop = get_or_reuse_loop() | ||
| result = loop.run_until_complete(func(*args, **kwargs)) | ||
| loop.close() |
There was a problem hiding this comment.
We can only close a loop, if we created it. If it was already running, we shouldn't.
There was a problem hiding this comment.
we should use some context manager here
There was a problem hiding this comment.
we could, its a bit tricky though. The underlying design issue is that the caller does not necessarily own the loop.
We could potentially make the new get_or_reuse_loop_with_info return a context manager instead of the tuple?
There was a problem hiding this comment.
I implemented this now in the helper and used here
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
Codecov Report
@@ Coverage Diff @@
## master #4734 +/- ##
==========================================
- Coverage 87.68% 87.62% -0.07%
==========================================
Files 119 119
Lines 8632 8670 +38
==========================================
+ Hits 7569 7597 +28
- Misses 1063 1073 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
jina/helper.py
Outdated
| return get_or_reuse_loop().run_until_complete(func(*args, **kwargs)) | ||
| loop = get_or_reuse_loop() | ||
| result = loop.run_until_complete(func(*args, **kwargs)) | ||
| loop.close() |
There was a problem hiding this comment.
we should use some context manager here
|
After discussion with @deepankarm we came up with a better solution which fixes the underlying issue: Not reusing existing loops. Lets see if it shows any undesired side effects in the tests. If not, lets use this solution. |
8f00d50 to
ede303d
Compare
|
actually there are some cases where we need to create a loop. We should not try to retrieve a running loop though. Any non closed loop is fine. |
| try: | ||
| loop = asyncio.get_running_loop() | ||
| loop = asyncio.get_event_loop() |
There was a problem hiding this comment.
this is the relevant change. We should use any loop (unless closed as checked in the next line).
|
I reset the loop now on forking due to this upstream issue |
We are leaking
asyncioloops inrun_ascynio. We useget_or_reuse_loop()there to create a newasyncioloop in each call. This loop is then used inrun_until_complete. This completes the coroutine, but does NOT close the loop.To fix the issue this PR now gets every existing loop. Not only the running loop as before. This will correctly reuse previously created loops. If there is no loop or of it is closed already, we create a new loop.
I also add
psutilas a new dependency for testing. I could also do its purpose manually, but it seems more convenient to use this library here and I dont think we need to be too restrictive for CI purposes?Fixes: #4725