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

thread safety checks #5120

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

ab9rf
Copy link
Member

@ab9rf ab9rf commented Dec 22, 2024

The main thing this PR does is add some code to monitor for attempts to acquire a CoreSuspender from the DF render thread, which is impermissible and will lead to deadlock. It currently only uses an assert for this (which means it's only operational in a debug build).

It also removes some dead code related to threading and fixes a couple of semantic errors:

  • Core::Shutdown attempts to unlock the main core lock, but cannot do so because it always runs on the render thread while the main core lock is held by the simulation thread. This is replaced by an atomic flag shutdown which Core::Shutdown sets and which Core::Update checks.
  • Plugin unloading would attempt to acquire a CoreSuspender to save plugin site and world data. Plugin unload during shutdown cannot acquire a CoreSuspender (because shutdown runs on the render thread), so unload now skips saving plugin site and world data during shutdown. This is harmless because site and world data cannot be saved during shutdown anyway.

also removed some dead code and fixed a problem with shutdown
@ab9rf ab9rf force-pushed the thread-safety-checks branch from cd6fe14 to 2c8ade8 Compare December 22, 2024 19:23
this avoids a deadlock risk that would otherwise arise
library/Core.cpp Outdated
// DF only calls the sdl input event hook from the render thread
// we use this to identify the render thread
//
// first time this is called, set df_render_thread to calling thread
Copy link
Member

Choose a reason for hiding this comment

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

stale comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, good catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

2 participants