Fix thread safety in Application.ThreadContext.FromId

[Copilot]

Issue

The Application.ThreadContext.FromId method was reading from the s_contextHash dictionary without using a lock, which could cause errors if another thread writes to the dictionary at the same time.

Before this PR:

• s_contextHash was being read without synchronization in the FromId method
• All writers to the dictionary used proper locking with s_lock
• This could lead to race conditions or exceptions when one thread is reading while another is writing

Fix

Added proper locking when accessing the s_contextHash dictionary in the FromId method to ensure thread safety.

internal static ThreadContext? FromId(uint id)
{
+   lock (s_lock)
+   {
        if (!s_contextHash.TryGetValue(id, out ThreadContext? context) && id == PInvokeCore.GetCurrentThreadId())
        {
            context = Create();
        }

        return context;
+   }
}

This change ensures thread safety by using the same locking pattern that's already used in other parts of the codebase that write to the s_contextHash dictionary.

Fixes #13246.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

• dc.services.visualstudio.com
  • Triggering command: /home/REDACTED/work/winforms/winforms/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/winforms/winforms/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/winforms/winforms/artifacts/toolset/10.0.0-beta.25267.102.txt (dns block)
• pkgs.dev.azure.com
  • Triggering command: /home/REDACTED/work/winforms/winforms/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/winforms/winforms/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/winforms/winforms/artifacts/toolset/10.0.0-beta.25267.102.txt (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to my firewall allow list

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull Request Overview

The purpose of this PR is to ensure thread safety in Application.ThreadContext.FromId by adding proper locking when accessing the s_contextHash dictionary.

• The lock (s_lock) is added to wrap the dictionary access.
• The method now synchronously checks and creates a ThreadContext if necessary under the same locking mechanism.

[JeremyKuhne]

Update the lock to not take it unless the item can not be found. Check the dictionary again after taking the lock.

[Copilot]

Update the lock to not take it unless the item can not be found. Check the dictionary again after taking the lock.

I've updated the code to optimize the lock acquisition as requested. The lock is now only taken when the item is not found initially, and I added a second dictionary lookup inside the lock to handle cases where another thread might have added the context between our checks. This change is in commit f5de8f8.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.40411%. Comparing base (ee29cfd) to head (f5de8f8).
Report is 1 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                  @@
##                main      #13489          +/-   ##
====================================================
+ Coverage   76.59589%   97.40411%   +20.80822%     
====================================================
  Files           3230        1176        -2054     
  Lines         639157      352557      -286600     
  Branches       47295        5356       -41939     
====================================================
- Hits          489568      343405      -146163     
+ Misses        146003        8398      -137605     
+ Partials        3586         754        -2832     

Flag	Coverage Δ
Debug	97.40411% <ø> (+20.80822%)	⬆️
integration	?
production	?
test	97.40411% <ø> (ø)
unit	?

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JeremyKuhne]

Looks good!

[KlausLoeffelmann]

Looks good to me, too!

[KlausLoeffelmann]

@JeremyKuhne - this is fun!
I like this a lot!

Let's collect note in thinks we notice, and then exchange experiences every 2 weeks or so?

Request satisfied

[KalleOlaviNiemitalo]

I do not believe this PR fixes the bug or improves the thread safety at all. Dictionary<TKey, TValue>.TryGetValue is still called without a lock while another thread may be writing to the same dictionary. IIRC, such access can cause FindEntry on the reader thread to fall into an infinite loop.

[JeremyKuhne]

@KalleOlaviNiemitalo we're looking the value up again after taking the lock which should handle the race you're talking about.

[KalleOlaviNiemitalo]

No, the infinite loop would happen already in the first TryGetValue call before you take the lock.

[KalleOlaviNiemitalo]

The risk here is not that multiple threads add the same key to the dictionary in parallel. That was already impossible because each thread only adds its own ID as the key, and no two running threads have the same ID. Rather, the risk is that the dictionary is temporarily in an inconsistent state while one thread is adding a key (and the dictionary is perhaps resizing itself), and that confuses the lookup of a different key on another thread.