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

Android/GCAdapter: Don't join current thread #13203

Merged
merged 1 commit into from
Dec 1, 2024

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Dec 1, 2024

The read thread could call Reset, which in turn tried to join the read thread, leading to a SIGABRT. This manifested as Dolphin consistently crashing when disconnecting a GC adapter and having a chance of crashing a few seconds after connecting a GC adapter.

The read thread could call Reset, which in turn tried to join the read
thread, leading to a SIGABRT. This manifested as Dolphin consistently
crashing when disconnecting a GC adapter and having a chance of crashing
a few seconds after connecting a GC adapter.
Copy link
Member

@jordan-woyak jordan-woyak left a comment

Choose a reason for hiding this comment

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

LGTM. I've not personally tested it on Android.

@OatmealDome OatmealDome merged commit 93c9424 into dolphin-emu:master Dec 1, 2024
13 checks passed
@Dentomologist
Copy link
Contributor

Is there anything preventing the following sequence of events?

  1. Setup() checks the first part of the condition on lines 526-527 s_read_adapter_thread_needs_joining.TestAndClear() || s_read_adapter_thread_running.TestAndClear()). s_read_adapter_thread_needs_joining hasn't been set yet, so TestAndClear() returns false.
  2. Reset() is called on the read thread and executes s_read_adapter_thread_needs_joining.Set(); and
    s_read_adapter_thread_running.Clear(); on lines 729 and 730.
  3. Setup() checks the second part of the condition on 527. Since s_read_adapter_thread_running just got cleared, that TestAndClear() also returns false.
  4. Since both parts of the conditional were false, Setup() doesn't join s_read_adapter_thread.
  5. s_read_adapter_thread = std::thread(ReadThreadFunc); on line 533. According to https://en.cppreference.com/w/cpp/thread/thread/operator%3D, if the read thread is still running this will call std::terminate.

@JosJuice JosJuice deleted the android-gcadapter-reset branch December 1, 2024 22:12
@JosJuice
Copy link
Member Author

JosJuice commented Dec 1, 2024

You're right, that sequence of events should be possible (albeit very unlikely). I'm going to try to think of a better design. For the time being, I'd prefer to keep this merged as it is rather than reverting it, since the current code has problems in a lot less cases than the previous code.

@JosJuice
Copy link
Member Author

JosJuice commented Jan 2, 2025

Addressed in PR #13260.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants