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

Device remains connected if connection is successful but get_services receives TimeoutError during connect #951

Closed
bdraco opened this issue Aug 20, 2022 · 5 comments · Fixed by #968 or #969
Labels
Backend: BlueZ Issues and PRs relating to the BlueZ backend

Comments

@bdraco
Copy link
Contributor

bdraco commented Aug 20, 2022

2022-08-20 11:12:01.209 DEBUG (MainThread) [yalexs_ble.lock] Front Door (M10CK0F): Disconnected from lock callback
2022-08-20 11:12:01.210 DEBUG (MainThread) [yalexs_ble.lock] Front Door (M10CK0F): Disconnected from lock callback
2022-08-20 11:12:01.211 DEBUG (MainThread) [yalexs_ble.lock] Front Door (M10CK0F): Disconnected from lock callback

I noticed I was getting the callback multiple times and tracked it down to the _remove_device_watcher not being canceled when the connection fails

self._remove_device_watcher = lambda: manager.remove_device_watcher(watcher)

@dlech dlech added the Backend: BlueZ Issues and PRs relating to the BlueZ backend label Aug 20, 2022
@dlech
Copy link
Collaborator

dlech commented Aug 26, 2022

I don't see how this is possible. Immediately after line 152, there is a try/except block that calls _cleanup_all on any exception. _cleanup_all calls self._remove_device_watcher().

How exactly is the connection failing?

@bdraco
Copy link
Contributor Author

bdraco commented Aug 26, 2022

It happens after the connection timed out. I expect the its raising CanceledError but I would have expected BaseException catching would have caught that and triggered the _cleanup_all

I have not yet been able to replicate it on demand.

@bdraco
Copy link
Contributor Author

bdraco commented Aug 26, 2022

So I think part of what's going on is (and this is a tricky race so I may still be missing something):

Disconnected Monitor is started

asyncio.ensure_future(self._disconnect_monitor())

await self.get_services() is called and the timeout from the code calling bleak's connect is hit, raising CanceledError

await self.get_services()

and then trapped by the BaseException so _cleanup_all() is called

self._cleanup_all()

Device completes connection in the background is is connected but we think it is not connected because the timeout was raised.

The next connection attempt things are in an indeterminate state since the device is actually connected but we don't know that.

@bdraco
Copy link
Contributor Author

bdraco commented Aug 26, 2022

So I think the race is that the cancel happens during get_services

Then _cleanup_all runs it calls self._bus.disconnect()

self._bus.disconnect()

And set self._bus to None

self._bus = None

After that the _disconnect_monitor runs and the bus is already disconnected so it leaves the connection open

await self._disconnect_monitor_event.wait()

@bdraco
Copy link
Contributor Author

bdraco commented Aug 26, 2022

And I just realized why I can't replicate it anymore.

Since I have get_services wrapped to disconnect on Exception it never happens anymore

https://github.com/Bluetooth-Devices/bleak-retry-connector/blob/acbee3824e4200f4acb43851ff90ccfd60ecb575/src/bleak_retry_connector/__init__.py#L160

@bdraco bdraco changed the title The _remove_device_watcher needs to be canceled when the connection failed Device remains connected if get_services receives TimeoutError Aug 28, 2022
@bdraco bdraco changed the title Device remains connected if get_services receives TimeoutError Device remains connected if connection is successful but get_services receives TimeoutError during connect Aug 28, 2022
@dlech dlech closed this as completed in b93082a Aug 30, 2022
@dlech dlech mentioned this issue Aug 31, 2022
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
Projects
None yet
2 participants