Ensure same lock is used to read and write Dictionary in AppContext #2729
Conversation
| isEnabled = overrideValue; | ||
|
|
||
| // Update the switch in the dictionary to mark it as 'checked for override' | ||
| SetSwitch(switchName, isEnabled); |
There was a problem hiding this comment.
I would just explicitly set the value here. Calling SetSwitch is going to go through a lot more checks and code before it sets the value.
|
@JonHanna thanks for doing this! Unfortunatelly, what you see here is not the entire code that uses AppContext and makes some assumptions about they way it is implemented. With that in mind, for this change, I would like to keep it as surgical as possible in order to not break some of the assumptions made by the rest of the code. I know it is difficult to fix something when you can't see all the parts. |
|
I think the code in that file should have been enough to realise that that was a bad commit. I broke my rule about always waiting a few minutes before clicking "Create Pull Request", and I never don't regret it when I break that rule. A more restrained update has been made. |
|
Thank you for the update! LGTM! I am going to work on getting some of our tests for AppContext into the open to try and help with validating the changes to it! |
| @@ -157,11 +156,13 @@ public static void SetSwitch(string switchName, bool isEnabled) | |||
| if (switchName.Length == 0) | |||
| throw new ArgumentException(Environment.GetResourceString("Argument_EmptyName"), "switchName"); | |||
|
|
|||
| lock (s_syncLock) | |||
| SwitchValueState switchValue = (isEnabled ? SwitchValueState.HasTrueValue : SwitchValueState.HasFalseValue) | |||
| | SwitchValueState.HasLookedForOverride | |||
There was a problem hiding this comment.
there is a missing trailing ';' which is causing the build break.
|
Test Ubuntu x64 Release Build and Test |
Ensure same lock is used to read and write Dictionary in AppContext
Fixes #2727