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.

Sign up for GitHub

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

Jump to bottom

input: Fix crash when closing add controller dialog before search completes #1386

Merged
merged 5 commits into from
Oct 28, 2024
Merged

input: Fix crash when closing add controller dialog before search completes #1386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f1ee49
input: Fix crash when closing add controller dialog before search com…
goeiecool9999 Oct 19, 2024
7ac0ed9
use lock_guard instead
goeiecool9999 Oct 19, 2024
8fa37f7
fix crash when closing before search, make discard a function for con…
goeiecool9999 Oct 20, 2024
9500000
use a jthread
goeiecool9999 Oct 20, 2024
d090a97
Revert "use a jthread"
goeiecool9999 Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 33 additions & 10 deletions src/gui/input/InputAPIAddWindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ InputAPIAddWindow::InputAPIAddWindow(wxWindow* parent, const wxPoint& position,
this->Bind(wxControllersRefreshed, &InputAPIAddWindow::on_controllers_refreshed, this);
}

InputAPIAddWindow::~InputAPIAddWindow()
{
discard_thread_result();
}

void InputAPIAddWindow::on_add_button(wxCommandEvent& event)
{
const auto selection = m_input_api->GetSelection();
Expand Down Expand Up @@ -159,6 +164,8 @@ std::unique_ptr<ControllerProviderSettings> InputAPIAddWindow::get_settings() co

void InputAPIAddWindow::on_api_selected(wxCommandEvent& event)
{
discard_thread_result();

if (m_input_api->GetSelection() == wxNOT_FOUND)
return;

Expand Down Expand Up @@ -239,19 +246,25 @@ void InputAPIAddWindow::on_controller_dropdown(wxCommandEvent& event)
m_controller_list->Append(_("Searching for controllers..."), (wxClientData*)nullptr);
m_controller_list->SetSelection(wxNOT_FOUND);

std::thread([this, provider, selected_uuid]()
m_search_thread_data = std::make_unique<AsyncThreadData>();
std::thread([this, provider, selected_uuid](std::shared_ptr<AsyncThreadData> data)
{
auto available_controllers = provider->get_controllers();

wxCommandEvent event(wxControllersRefreshed);
event.SetEventObject(m_controller_list);
event.SetClientObject(new wxCustomData(std::move(available_controllers)));
event.SetInt(provider->api());
event.SetString(selected_uuid);
wxPostEvent(this, event);

m_search_running = false;
}).detach();
{
std::lock_guard lock{data->mutex};
if(!data->discardResult)
{
wxCommandEvent event(wxControllersRefreshed);
event.SetEventObject(m_controller_list);
event.SetClientObject(new wxCustomData(std::move(available_controllers)));
event.SetInt(provider->api());
event.SetString(selected_uuid);
wxPostEvent(this, event);
m_search_running = false;
}
}
}, m_search_thread_data).detach();
}

void InputAPIAddWindow::on_controller_selected(wxCommandEvent& event)
Expand Down Expand Up @@ -301,3 +314,13 @@ void InputAPIAddWindow::on_controllers_refreshed(wxCommandEvent& event)
}
}
}

void InputAPIAddWindow::discard_thread_result()
{
m_search_running = false;
if(m_search_thread_data)
{
std::lock_guard lock{m_search_thread_data->mutex};
m_search_thread_data->discardResult = true;
}
}
9 changes: 9 additions & 0 deletions src/gui/input/InputAPIAddWindow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class InputAPIAddWindow : public wxDialog
{
public:
InputAPIAddWindow(wxWindow* parent, const wxPoint& position, const std::vector<ControllerPtr>& controllers);
~InputAPIAddWindow();

bool is_valid() const { return m_type.has_value() && m_controller != nullptr; }
InputAPI::Type get_type() const { return m_type.value(); }
Expand All @@ -38,6 +39,8 @@ class InputAPIAddWindow : public wxDialog
void on_controller_selected(wxCommandEvent& event);
void on_controllers_refreshed(wxCommandEvent& event);

void discard_thread_result();

wxChoice* m_input_api;
wxComboBox* m_controller_list;
wxButton* m_ok_button;
Expand All @@ -50,4 +53,10 @@ class InputAPIAddWindow : public wxDialog

std::vector<ControllerPtr> m_controllers;
std::atomic_bool m_search_running = false;
struct AsyncThreadData
{
std::atomic_bool discardResult = false;
std::mutex mutex;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Make the mutex a member variable of InputAPIAddWindow. Tracking it per-thread only unnecessarily complicates things and is error prone.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this may then introduce a race condition where this is destroyed before the thread accesses it. Not sure, maybe there is a less fragile way to handle this in general.

Copy link
Collaborator Author

@goeiecool9999 goeiecool9999 Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

maybe there is a less fragile way to handle this in general.

Have a look at the latest commit. I switched to a jthread. I think I was reinventing the wheel.
EDIT: I just remembered that Apple Clang doesn't support std::jthread 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've reverted it. Not only because of the lack of macOS support but also because it has more theoretical race conditions than the mutex.

};
std::shared_ptr<AsyncThreadData> m_search_thread_data;
};
Loading