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

ALSA plugin: Reduce mutex lock duration #57

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

Conversation

NexAdn
Copy link

@NexAdn NexAdn commented Oct 22, 2022

To mitigate deadlock issues on switching the audio input device, this commit reduces the time m_deviceListMutex is locked by switching std::lock_guard and manual locking for std::unique_lock and deferring locking only to places where access to the device list is needed.

Important note: During testing I sometimes had issues where the RTA and sidebar level meter didn't work correctly with some input configurations (especially the time window and input source settings). But since I am unfamiliar with the codebase, I can't tell if this issue is related.

Closes: #56

To mitigate deadlock issues on switching the audio input device, this
commit reduces the time m_deviceListMutex is locked by switching
std::lock_guard and manual locking for std::unique_lock and deferring
locking only to places where access to the device list is needed.

Closes: psmokotnin#56
@NexAdn
Copy link
Author

NexAdn commented Oct 22, 2022

Regarding the display issues: I was just able to fix that by using a different audio interface. But I guess that's is unrelated to the original issue that this PR fixes.

@NexAdn
Copy link
Author

NexAdn commented Dec 1, 2022

Any updates on this?

@psmokotnin
Copy link
Owner

@NexAdn Thanks for the PR. Sorry was very loaded the last time. This issue requires a more detailed investigation and the solution also.

@NexAdn
Copy link
Author

NexAdn commented Dec 23, 2022

Well, from what the name suggests, m_deviceListMutex is responsible for handling access to m_devices,
so passing the mutex on to the AlsaPCMDevice is not a good idea in the first place.
Otherwise, I'd consider this variable poorly named as it seems to cover things beyond its name, while the name is pretty specific in that the mutex is there to serialize access to the device list, not the devices.
Another thing to consider is that keeping a mutex for longer than it's actually needed (which normally boils down to accessing a shared resource) is almost always a bad idea as it leads to problems like this.

I've now been using this fix for quite a while without any problems.
The semantics of your code seem work out perfectly fine to ensure your devices aren't deleted before they are finished doing their things.
Thus, I don't think there is any reason to keep any locks for a long time.

If you think that my understanding of your locking mechanism here is wrong, please let me know. Then I can have a fresh look at it, if you lack the time or resources to do so.

@psmokotnin
Copy link
Owner

This PR doesn't solve the issue, I'll need more time to investigate the real reason for this deadlock and apply changes to this PR or I'll make a new fix for this issue.
AudioPlugin tells the application what devices are now available, that's why it requires this mutex.

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.

Deadlock in ALSA plugin
2 participants