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

Threading fixes for config layers #8273

Open
wants to merge 2 commits into
base: master
from

Conversation

@CookiePLMonster
Copy link
Contributor

commented Jul 30, 2019

This PR refactors Config Layouts functionality a bit, adding a layer of thread safety to them, as well as improving overall clarity. This fixes several "random" crashes on switching the game on/off and accessing configuration menus at the same time.

  1. First commit revisits layer manager in Config namespace to make it thread safe. API has been made a bit stricter (Config::GetLayers was thankfully never used, though!) and some implementation details have been moved from the header file to the .cpp file. Now configuration layers are stored in a shared_ptr and not an unique_ptr, which cleanly solves an issue of one thread reading those, with another thread deleting them on terminating the game.
    On top of that, any functions accessing s_layers have been protected with a read-write lock. It might seem like I added some redundant scopes, but that's not the case - InvokeConfigChangedCallbacks must not be protected with this mutex, else deadlocks occur!
  2. Second commit revisits LayerMap. Previously, it was defined as std::map<ConfigLocation, std::optional<std::string>> and presence of a nullopt value was treated equally to lack of a value. I was able to remove this redundancy by replacing some uses of operator[] with find(). This also allowed me to mask Get functions as const, which was not possible before due to an implicit construction in operator[]. Additionally, I believe this may be a memory/performance improvement, since config layers will now not store redundant, empty entries for options they were probed on.
    EDIT: This has been partially reverted to again store optionals in a map, since it's needed to achieve correct behaviour of deleting config keys. Fixes around operator[] persisted.

Races fixed

I was unable to reproduce this race in Release mode, most likely due to very specific timing required to reproduce it. However, I was able to reproduce it in Debug:

  1. Open Graphics options.
  2. Launch the game, and quickly terminate it.
  3. Repeat 2) until Dolphin crashes.

There were numerous reasons for those crashes - either one thread was reading from a Layer which was already deleted by another thread (that's what usage of std::shared_ptr fixes), or map's iterators get corrupted because one thread was iterating through it while the other was adding/removing entries to it (that's what a read-write lock fixes).

NOTE: There seems to be at least one more race on starting the game (GetDSPProcessor() returning null and being used without any validation), but this will not be fixed by this PR. If I find a fix better than "add a null check beforehand", I'll submit another PR.

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:config-threading-fixes branch from be41f69 to f42acd1 Jul 30, 2019

Source/Core/Common/Config/Config.cpp Outdated Show resolved Hide resolved
Source/Core/Common/Config/Config.cpp Outdated Show resolved Hide resolved
Source/Core/Common/Config/Config.cpp Outdated Show resolved Hide resolved
Source/Core/Common/Config/Config.cpp Outdated Show resolved Hide resolved
Source/Core/Common/Config/Layer.cpp Outdated Show resolved Hide resolved
Source/Core/Common/Config/Layer.h Outdated Show resolved Hide resolved
Source/Core/Common/Config/Layer.h Outdated Show resolved Hide resolved
Source/Core/Common/Config/Config.cpp Outdated Show resolved Hide resolved
Source/Core/Common/Config/Config.cpp Outdated Show resolved Hide resolved
Source/Core/Common/Config/Config.cpp Outdated Show resolved Hide resolved
Fix race conditions in Config Layers
API has been made stricter, layers are now managed with shared pointers,
so using them temporarily increased their reference counters.
Additionally, any s_layers map has been guarded by a read/write lock,
as concurrent write/reads to it were possible.

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:config-threading-fixes branch from f42acd1 to 6f6a819 Jul 30, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Done, also fixed ClearCurrentRunLayer, it was still using std::make_unique despite migrating to shared_ptr.

@leoetlino

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Won't your second change (not using std::optional) re-introduce an issue where calling DeleteKey will not actually delete settings from the underlying IniFile? (#6154)

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Hmmmm, good point - I focused only on what Layer.cpp was doing with those values but didn't check the rest. It's indeed possible that it'll reintroduce that, how do I verify that best?

EDIT:
Even if that's the case, only a partial revert is appropriate I think - as current implementation is also prone to creating lots of potentially redundant entries solely by having them probed by Get methods.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

OK so I've been able to verify that both Layer::DeleteKey and Layer::DeleteAllKeys are unused, Dolphin compiles fine with them removed. Therefore, I see two ways out of this:

  1. Remove deleting functionality as it's not needed now.
  2. Partially revert my change to still use std::optional<std::string>, but without reverting exceessive use of operator[] which created entries even in getters.
@leoetlino

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

I think 2 would be the best solution here; partially revert but keep the getter fixes. The Delete functions will likely be used as more parts of the codebase are migrated over to the new config system.

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:config-threading-fixes branch from 6f6a819 to 41f1d00 Jul 31, 2019

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:config-threading-fixes branch from 41f1d00 to 48a4b62 Aug 1, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

I just noticed the commit message did not reflect the current state of this code - so I resubmitted with an updated message so git history isn't polluted by false claims.

@BhaaLseN
Copy link
Member

left a comment

Code looks good I guess, untested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.