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

SaveManager cleanup #3386

Merged

Conversation

Malkierian
Copy link
Contributor

@Malkierian Malkierian commented Nov 12, 2023

Move thread pool initialization and OnExitGame registration from SaveManager::Init() to SM's constructor.

This is necessary because SaveManager::Init() is merely an extension of Sram_InitSram(), and as such is not an initializer for SaveManager itself. Before now, every reset would add another registration for OnExitGame and reinitialize the thread pool, which was definitely not a good thing.

Also added call to ThreadPoolWait() in Init(), as it was relatively easy, especially on Anchor, to trigger a save after OnExitGame was already processed, thus introducing the possibility that Init() would be loading a file (or reinitializing the thread pool, for that matter) at the same time the save worker was writing to the file, causing it to try to load incomplete and malformed save data.

Build Artifacts

…veManager::Init` to SM's constructor.

Comment on `Init` to mention it's not an initializer for `SaveManager`.
Added check for `SaveManager::SaveSection` to prevent firing a save worker if the game is already exited from a reset.
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

left a non-blocking nitpick comment

soh/soh/SaveManager.cpp Outdated Show resolved Hide resolved
@briaguya-ai briaguya-ai self-requested a review November 12, 2023 07:13
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

i misunderstood what was happening with my prior approval, and the code has changed since then, this is just to make my green check go away (i'll re-review later)

Copy link
Contributor

@Archez Archez left a comment

Choose a reason for hiding this comment

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

Changes look reasonable to me (class initializers performed in the constructor).

@briaguya-ai briaguya-ai merged commit ba987c4 into HarbourMasters:develop-macready Nov 14, 2023
8 checks passed
@Malkierian Malkierian deleted the save-manager-cleanup branch November 14, 2023 21:47
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.

None yet

3 participants