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

WIP: Fix notification unsubscribe lock #294

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

Conversation

Electro707
Copy link

Here is a description of the cause of this pull request (I will be referring to the bgapi backend).

Description of issue:

When the unsubscribe function tries to unsubscribe from a certain notification, it locks a thread lock with with self._lock. The unsubscribe function then calls a function which waits for a 'write confirmation' data packet from the Bluetooth client to confirm that the write to the notification configuration handler is successful. If a server is notifying the client at a fast enough pace, a notification packet will come it before the 'write confirmation' packet. That notification packet will then call the _receive_notification function. That function can't run as the _lock lock is locked up by the subscribe function. This results in a lockup, as the _receive_notification function is trying to use a lock that is occupied and won't be released until the subscribe function receives a 'write confirmation' packet, which it can't as the packet handler is locked up by the _receive_notification function.

(Hopefully I got my point across and didn't create more confusion than before)

Fix:

A solution is to have the _receive_notification function check if either:

  • The _lock lock is acquired by another function (What this PR currently does as of commit 60b7ed4)
  • Check if there is the handle it wants in the self._callbacks array before the with self._lock line is called.

What is the best way to approach this issue (or if there is another better way to)?

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.

1 participant