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

FIX: do not create a new event loop so often #1718

Closed
wants to merge 1 commit into from

Conversation

tacaswell
Copy link
Contributor

Partially reverts #1714

@carlwgeorge
Copy link

This looks to resolve #1719.

@jonathanslenders
Copy link
Member

Thank you, but I don't really like the global.

The problem here is that we have a situation where an event loop was set, but is not running, and calling get_running_loop() fails. We could call get_event_loop() which would return the event loop that was previously set, but that emits a warning on newer Python versions. So, if we call get_running_loop(), and that fails, after which we create and set a new loop, that will leak file descriptors from the previous loop.

I'll look into using asyncio.run() instead, and dropping Python 3.6. I think that's fine.

@tacaswell
Copy link
Contributor Author

As near as I can tell, there is no way to ask asyncio "do you have a set but not running event loop?" as all flavors of get_event_loop have both the auto-creation (+ warning) baked in. I do not think making new loops is so much the problem, but failing to close them (which leaves some pipes open)

I think the options (other than switching to asyncio.run()) are all not great:

  • this (which will leak up to one event loop and if the user sets the loop back to None will still produce the warning which can happen if asyncio.run() is called)
  • fully own a per-thread event loop cache (which might be the second best option overall as upstream really wants to hide the loop object from code outside of the loop and it may go away in the future all together). However, there would then be two caches, one of which is being touched by other code and no way to check if the cache has gone out of sync without risking the warning and if you just force it then you will leak event loops
  • reach in and access the private API for asyncio's cache (bad for obvious reasons)
  • just eating the warning (bad because it will eventually break for real)

@tacaswell
Copy link
Contributor Author

tacaswell commented Feb 1, 2023

I see #1721 was opened so I'm going to close this.

Sorry for the noise!

@tacaswell tacaswell closed this Feb 1, 2023
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.

3 participants