TaskFactoryWrapper: guard against multi-threaded access to dictionaries

[jdrobison]

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1824802

This change protects the dictionaries owned by TaskFactoryWrapper from being mutated while being accessed by multiple threads.

• Change TaskFactoryWrapper's dictionary fields from IDictionary<K,V> to IReadOnlyDictionary<K,V>
• Add support for IReadOnlyDictionary<K,V> to ReadOnlyEmptyDictionary
• Ensure mutually exclusive access to TaskFactoryWrapper.PopulatePropertyInfoCacheIfNecessary, which is the place where all of the object's dictionaries are created.

[jdrobison]

The PR checks are showing failures in Microsoft.Build.CommandLine.UnitTests. I can repro those failures locally, but I can also repro them in the main branch.

Are these known failures that should not block completion of this PR?

[rainersigwald]

Are these known failures that should not block completion of this PR?

Yes, should be fixed by #8927. I'll rerun here to confirm.

[rainersigwald]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JanKrivanek]

Thanks!

[jdrobison]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 8928 in repo dotnet/msbuild

[jdrobison]

@rainersigwald / @JanKrivanek - it looks like there might be an infra issue that caused the msbuild-pr (Windows Full) check to fail. I don't have sufficient perms to re-run the checks. Could one of you re-run them?

D:\a\1\s\artifacts\toolset\restore.proj : error : Could not resolve SDK "Microsoft.DotNet.Arcade.Sdk". Exactly one of the probing messages below indicates why we could not resolve the SDK. Investigate and resolve that message to correctly specify the SDK.
D:\a\1\s\artifacts\toolset\restore.proj : error :   D:\a\1\s\.dotnet\sdk\7.0.304\Sdks\Microsoft.DotNet.Arcade.Sdk\Sdk not found. Check that a recent enough .NET SDK is installed and/or increase the version specified in global.json.
D:\a\1\s\artifacts\toolset\restore.proj : error :   Unable to find package Microsoft.DotNet.Arcade.Sdk with version (= 6.0.0-beta.23313.5)
D:\a\1\s\artifacts\toolset\restore.proj : error :   - Found 178 version(s) in dotnet-tools [ Nearest version: 5.0.0-beta.19558.13 ]
D:\a\1\s\artifacts\toolset\restore.proj : error :   - Found 0 version(s) in arcade
D:\a\1\s\artifacts\toolset\restore.proj : error :   - Found 0 version(s) in dotnet-public
D:\a\1\s\artifacts\toolset\restore.proj : error :   - Found 0 version(s) in dotnet6
D:\a\1\s\artifacts\toolset\restore.proj : error :   MSB4276: The default SDK resolver failed to resolve SDK "Microsoft.DotNet.Arcade.Sdk" because directory "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Sdks\Microsoft.DotNet.Arcade.Sdk\Sdk" did not exist.
D:\a\1\s\artifacts\toolset\restore.proj : error : Unable to load the service index for source https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json.
D:\a\1\s\artifacts\toolset\restore.proj : error MSB4236: The SDK 'Microsoft.DotNet.Arcade.Sdk' specified could not be found.

[JanKrivanek]

FYI @rokonec - this is similar situation as we discussed over #8861 - the concurrently accessed structure (TaskFactoryWrapper in this case) is only accessed from RequestBuilder - so technically should not be accessed only sequentially.
I hope this is caused by the same rootcause (TaskRegistry being only shallow copied during copying of ProjectInstance) - as the TaskFactoryWrapper is fetched from TaskRegistry here: https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs#L251 - so technically unintended sharing of TaskFactoryWrapper can happen

[jdrobison]

@rainersigwald, could I get another approval on this PR? Thanks.

[rainersigwald]

@jdrobison we're hoping to solve the problem a different way so we're not ready to accept (or reject) this yet.

[JanKrivanek]

@jdrobison appology for the confusion on this ticket - we've been looking deeper into few seemingly unrelated tickets manifesting multithreaded usage of TaskRegistry, that is not expected to be used in multithreaded context - and addressed this by preventing unintended sharing

[JanKrivanek]

superseded by #8973

[jdrobison]

@JanKrivanek - understood, thanks.

[JanKrivanek]

@jdrobison - Can you restore your bug/1824802.guard-TaskFactoryWrapper-dictionaries branch so that we can reopen this PR?

Upon detailed investigation of our TaskRegistry and calling code we've come to conslusion that it actually should not be cloned, but rather made thread safe - which by extension applies to TaskFactoryWrapper returned by the TaskFactory.

So we want to take your original fix

[jdrobison]

@JanKrivanek - my branch has been restored, but I'm not sure how to reactivate the PR. Suggestions welcome.

[rainersigwald]

(The button was way down below "comment")

[jdrobison]

Thanks, @rainersigwald!

[AR-May]

LGTM