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

ControllerInterface: fix UpdateReferences() deadlock #10228

Merged
merged 1 commit into from Nov 23, 2021

Conversation

Filoppi
Copy link
Contributor

@Filoppi Filoppi commented Nov 17, 2021

Removed useless locks to DeviceContainer::m_devices_mutex, as they were all already protected by m_devices_population_mutex.
We have no interest in blocking other threads that were potentially reading devices at the same time so this seems fine.
This simplifies the code, and I've adjusted a few comments which mentioned possible deadlock that should now be totally gone.

The deadlock could have happen if a thread directly called EmulatedController::UpdateReferences(), while another another thread also reached EmulatedController::UpdateReferences() within a call to ControllerInterface::UpdateDevices(), as the mentioned function locked both the DeviceContainer::m_devices_mutex and s_get_state_mutex at the same time.

The deadlock was frequent on game emulation startup on Android, due to the UpdateReferences() call in InputConfig::LoadConfig() and the UI thread triggering calls to ControllerInterface::UpdateDevices().
It could also have happened on Desktop if a user pressed "Refresh Devices" manually in the UI while the input config was loading.

Also brought some UpdateReferences() comments and thread safety fixes from dolphin-emu#9489

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not fix the deadlock when tested on Android.

@Filoppi Filoppi force-pushed the fix_android_input_deadlock branch 2 times, most recently from aaea3ef to e72b96e Compare November 18, 2021 00:05
@JosJuice JosJuice dismissed their stale review November 20, 2021 11:50

The current version does not show any deadlocks when tested

Removed useless locks to DeviceContainer::m_devices_mutex, as they were all already protected by m_devices_population_mutex.
We have no interest in blocking other threads that were potentially reading devices at the same time so this seems fine.
This simplifies the code, and I've adjusted a few comments which mentioned possible deadlock that should now be totally gone.

The deadlock could have happen if a thread directly called EmulatedController::UpdateReferences(), while another another thread also reached EmulatedController::UpdateReferences() within a call to ControllerInterface::UpdateDevices(), as the mentioned function locked both the DeviceContainer::m_devices_mutex and s_get_state_mutex at the same time.

The deadlock was frequent on game emulation startup on Android, due to the UpdateReferences() call in InputConfig::LoadConfig() and the UI thread triggering calls to ControllerInterface::UpdateDevices().
It could also have happened on Desktop if a user pressed "Refresh Devices" manually in the UI while the input config was loading.

Also brought some UpdateReferences() comments and thread safety fixes from dolphin-emu#9489
Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change makes sense, but I must say that all this locking is pretty tricky to reason about, both before and after the change. Hesitant LGTM (purely due to my inexperience with this part of the codebase).

What I am fairly sure about is that this fixes the bug on Android.

@Filoppi
Copy link
Contributor Author

Filoppi commented Nov 20, 2021

@JosJuice I looked at this very carefully and I'm fairly sure this only have benefits.
The reason for the code being like it was, meaning having unnecessary m_devices_mutex locks within m_devices_population_mutex locks that already made reading and writing devices 100% safe, is probably that it was the first step I took to make the ControllerInterface more thread safe, then I introduced the m_devices_population_mutex, and I assume I forgot that the additional locks to m_devices_mutex were now unnecessary, so instead of removing them, I tried to work around them (this wasn't the only deadlock they had caused, as the comments showed, but I had managed to fix all the others).

I've also carefully looked at #9489 to see if my input branch actually needed these extra m_devices_mutex locks, and the answer is no.

In any case, I'm here to fix anything on the stuff I developed.

This is a previous version of this PR, which fixed the problem but added complexity, and wasn't really the right approach:
#10234
and this is another alternative fix:
#10226
they should both work.

@leoetlino leoetlino merged commit e1e3db1 into dolphin-emu:master Nov 23, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants