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 DBus connection leak #1699

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

koenvsteenb
Copy link

@koenvsteenb koenvsteenb commented Dec 16, 2024

Moved the try (in bluezdbus/client.py) in order to include the call to the add_device_watcher function, since it is able to throw an error when the device does not exists in bluez anymore. This causes the just opened dbus connection to not be properly closed.

fixes #1698

Pull Request Guidelines for Bleak

Before you submit a pull request, check that it meets these guidelines:

  1. If the pull request adds functionality, the docs should be updated.
  2. Modify the CHANGELOG.rst, describing your changes as is specified by the
    guidelines in that document.
  3. The pull request should work for Python 3.8+ on the following platforms:
    • Windows 10, version 16299 (Fall Creators Update) and greater
    • Linux distributions with BlueZ >= 5.43
    • OS X / macOS >= 10.11
  4. Squash all your commits on your PR branch, if the commits are not solving
    different problems and you are committing them in the same PR. In that case,
    consider making several PRs instead.
  5. Feel free to add your name as a contributor to the AUTHORS.rst file!

Moved the try in order to include the call to the add_device_watcher function, since it is able to throw an error when the device does not exists in bluez anymore. This causes the just opened dbus connection to not be properly closed.
@dlech
Copy link
Collaborator

dlech commented Dec 16, 2024

Thanks for the fix. I wonder if we could improve this even more with a contextlib.AsyncExitStack .

@koenvsteenb
Copy link
Author

You mean something like this? My python skills are not that great, so it could be nothing. And I'm not sure if I understand the contextlib.AsyncExitStack correctly.

    @asynccontextmanager
    async def _connect_bus(self):
        self._bus = await MessageBus(
            bus_type=BusType.SYSTEM,
            negotiate_unix_fd=True,
            auth=get_dbus_authenticator(),
        ).connect()

        try:
            yield
        finally:
            self._cleanup_all()

    def connect():
        # ...rest of the code...
        async with async_timeout(timeout), AsyncExitStack() as stack:
            while True:
                # Each BLE connection session needs a new D-Bus connection to avoid a
                # BlueZ quirk where notifications are automatically enabled on reconnect.
                await stack.enter_async_context(self._connect_bus())

Also I'm unsure if this means that in some places the _cleanup_all() call can be removed.

@dlech
Copy link
Collaborator

dlech commented Dec 17, 2024

Sort of. But AsyncExitStack also has a push_async_callback() method, so we don't need to make a context manager for the cleanup fucntion. And at the very end of the function, we need to call stack.pop_all() so that the cleanup doesn't run on successful return.

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.

DBUS connection not closed properly in all cases
2 participants