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

backends/bluezdbus: use an a new reactor for each client #143

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

mickael9
Copy link
Contributor

@mickael9 mickael9 commented Jan 6, 2020

This makes things much cleaner :

  • The default asyncio event loop is not modified
  • No default twisted reactor is installed (a new one will be created every time)
  • The reactor will use the provided event loop

Related to: #93 #125

This makes things much cleaner :
- The default asyncio event loop is not modified
- No default twisted reactor is installed
- The reactor will use the provided event loop
@hbldh hbldh self-assigned this Jan 6, 2020
@hbldh hbldh added Backend: BlueZ Issues and PRs relating to the BlueZ backend enhancement New feature or request labels Jan 6, 2020
@hbldh hbldh added this to the v0.5.2 milestone Jan 6, 2020
@hbldh
Copy link
Owner

hbldh commented Jan 6, 2020

This. This is how it should have been from the start. Thank you very much, this I will merge immediately!

@hbldh hbldh merged commit 404869f into hbldh:develop Jan 6, 2020
@mickael9
Copy link
Contributor Author

mickael9 commented Jan 6, 2020

Thanks. I haven't got much experience with asyncio and multiple event loops. I'm still wondering exactly how useful the "loop" argument is, since we're already in an async function, there must already be a running event loop, and I don't think you can await on tasks from another event loop

@hbldh
Copy link
Owner

hbldh commented Jan 6, 2020

If no loop is specified, the default loop will be used. There are multiple people you have asked about sending in non-default loops, This enables running multiple BleakClients in the same process, which is highly desireable.

hbldh added a commit that referenced this pull request Jan 6, 2020
This tries to ensure that reactor is closed and deleted after client disconnects, to fix #130
hbldh added a commit that referenced this pull request Jan 6, 2020
This tries to ensure that reactor is closed and deleted after client disconnects, to fix #130
JoeHut pushed a commit to workaroundgmbh/bleak that referenced this pull request Feb 4, 2020
This tries to ensure that reactor is closed and deleted after client disconnects, to fix hbldh#130
@JoeHut
Copy link
Contributor

JoeHut commented Feb 4, 2020

actually this is problematic... It looks like the reactor leaks file descriptors (2-3 for each connected client). So if you are connecting and disconnecting devices continuously you will run out of file descriptors and crash. I am not really familiar with twisted, but as I understand it, there are wakers installed represented by file descriptors and neither closed by calling stop(), disconnectAll() or removeAll() nor affected by the garbage collector... So I guess the only way is to try and instantiate as few reactors as possible and pass them. If there is someone who can explain me more, I would be happy to help

@JoeHut
Copy link
Contributor

JoeHut commented Feb 5, 2020

The only reference I found is this really old ticket: https://twistedmatrix.com/trac/ticket/3063
But I don't quite understand whether it was resolved or what the general development state of twisted is

JoeHut pushed a commit to workaroundgmbh/bleak that referenced this pull request Feb 5, 2020
This tries to ensure that reactor is closed and deleted after client disconnects, to fix hbldh#130
@hbldh hbldh mentioned this pull request Mar 8, 2020
hbldh added a commit that referenced this pull request Mar 9, 2020
This tries to ensure that reactor is closed and deleted after client disconnects, to fix #130
@hbldh hbldh mentioned this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend: BlueZ Issues and PRs relating to the BlueZ backend enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants