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

Concurrency Fix #4318

Merged
merged 2 commits into from
Sep 15, 2024
Merged

Concurrency Fix #4318

merged 2 commits into from
Sep 15, 2024

Conversation

Malkierian
Copy link
Contributor

@Malkierian Malkierian commented Sep 2, 2024

Removed all CVarLoad uses from code to prevent thread concurrency issues.
Also added mutext locking to prevent concurrent save and load operations on the save file.

Build Artifacts

Copy link
Contributor

@Pepe20129 Pepe20129 left a comment

Choose a reason for hiding this comment

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

Fixes #4299.

@Pepper0ni
Copy link
Contributor

Is there anything that needs testing for this aside from building seeds until I'm sure cvar crash is gone?

@Malkierian
Copy link
Contributor Author

Not that I can think of. To be fair, I think I need to change the exit and reload locking to utilize the mutex as well before merging, but it's really hard to test either of those things if you're not on a console. The CVar thing was the only one that was very noticeable on PC.

Copy link
Contributor

@Pepper0ni Pepper0ni left a comment

Choose a reason for hiding this comment

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

Well no CVar crashes in a dozen seeds when I plugged the changes into RefactorGAL while testing it, so that's good enough for me.

@Malkierian
Copy link
Contributor Author

Malkierian commented Sep 13, 2024

And, for the record, after checking, utilizing the mutex in restart and exit shouldn't be necessary. I tried timing both in debug and couldn't get a single crash or corrupted save file as the system stands now.

@Malkierian
Copy link
Contributor Author

I don't know what the exit routines are like for Wii U and Switch closing the app from their menus. It could be a matter of those processes just killing the apps, in which case we might want to add somewhere to quit via the SoH menu instead to ensure save integrity.

@Archez Archez merged commit 7f503c3 into HarbourMasters:develop Sep 15, 2024
5 checks passed
@Malkierian Malkierian deleted the concurrency branch September 15, 2024 05:21
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.

4 participants