Workspaces: Allow uncontended waits without multithreading - #85869
pavelsavara wants to merge 1 commit into
Conversation
Try an immediate asynchronous acquisition before falling back to the existing synchronous wait. This allows single-threaded runtimes to acquire available semaphores while preserving synchronous waiter priority under contention. Related to dotnet/runtime#134972 and dotnet#84615. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
dotnet-policy-service
Bot
added
Community
| semaphore.Wait(cancellationToken); | ||
| // WaitAsync supports an uncontended synchronous acquisition on single-threaded runtimes. Fall back | ||
| // to Wait under contention to preserve synchronous waiter priority and blocking behavior. | ||
| if (!semaphore.WaitAsync(0, cancellationToken).GetAwaiter().GetResult()) |
There was a problem hiding this comment.
this isn't a sustainable pattern. if you want single-threaded-runtimes to work, then having them implement Wait(ct) using this pattern is fine. We're not going to chase having to patch up any and all places in the code that have an issue like this. THis is also a major footgun as it would be trivial to fall into breaking something on those runtimes at any point in teh future.
There was a problem hiding this comment.
@pavelsavara Does this pattern even solve the problem with contended semaphore on single threaded runtime? I would expect it to hang or crash (same as synchronous Wait).
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
this isn't a sustainable pattern. if you want single-threaded-runtimes to work, then having them implement Wait(ct) using this pattern is fine. We're not going to chase having to patch up any and all places in the code that have an issue like this. THis is also a major footgun as it would be trivial to fall into breaking something on those runtimes at any point in teh future.
We started throwing the PNSE in Net11, in order to signal that the We could easily (partially) revert that change. The problem is that the application code, like in this PR, can assume that the API just works (anyway, ignoring the UnsupportedOSPlatform). But it only works when the semaphore is uncontended. When the semahore has (probably async) contention, the behavior is UB. So throwing PNSE forces the application code to re-think if they support ST (browser) or not. |
Finding issues with the single-threaded browser model through exploration is not a predictable or scalable experience. Instead, libraries that want to support this model should run the browser compatibility analyzer. This follows the same playbook we use for trimming and AOT compatibility: the analyzer identifies all potentially problematic locations up front, giving library maintainers a concrete list to review. They can then evaluate those issues and decide whether supporting the form factor makes sense for their library.
I would be ok with that. Again, it is same as what we do for trimming and AOT compatibility in some places. We have APIs that are marked as trimming or AOT incompatible and the docs describe the conditions under which the APIs work. The developers are expected to review their code and disable the compatibility warning with a comment that explains why the specific use is fine. |
This branch has not been deployed
3 participants
Summary
DisposableWaithelpersMotivation
SemaphoreSlim.Waitis unsupported on single-threaded runtimes even when the semaphore is immediately available. Roslyn workspace operations use internal synchronous wrappers aroundSemaphoreSlim, which prevented uncontended operations such asAdhocWorkspace.AddSolutionandAddProjectfrom running on single-threaded Browser and WASI.The zero-timeout
WaitAsyncfast path completes synchronously when the semaphore is available. If it is unavailable, the code falls back to the existing synchronous wait so multithreaded contention and synchronous waiter priority remain unchanged.No public API is changed.
Validation
dotnet build src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj -p:RunAnalyzersDuringBuild=truedotnet build src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Microsoft.CodeAnalysis.Razor.Workspaces.csproj -p:RunAnalyzersDuringBuild=truedotnet test src/Workspaces/CoreTest/Microsoft.CodeAnalysis.Workspaces.UnitTests.csproj --filter "FullyQualifiedName~AdhocWorkspaceTests|FullyQualifiedName~ProjectDependencyGraphTests"dotnet test src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.UnitTests/Microsoft.CodeAnalysis.Razor.Workspaces.UnitTests.csproj --filter "FullyQualifiedName~RazorCodeDocumentExtensionsTest"dotnet test src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests.csproj --filter "FullyQualifiedName~HtmlDocumentSynchronizerTest"git diff --checkDirect Browser/WASI execution was not run.
Related: dotnet/runtime#134972
Resolves #84615
Note
This pull request was prepared with assistance from GitHub Copilot.
Microsoft Reviewers: Open in CodeFlow