Skip to content

[wasm][coreclr] Disable CircularCctor multithreading tests on single-threaded WASM#126120

Merged
radekdoulik merged 2 commits intomainfrom
wasm-disable-circularcctor-threading-tests
Mar 26, 2026
Merged

[wasm][coreclr] Disable CircularCctor multithreading tests on single-threaded WASM#126120
radekdoulik merged 2 commits intomainfrom
wasm-disable-circularcctor-threading-tests

Conversation

@radekdoulik
Copy link
Member

Note

This PR was created with Copilot assistance.

Add ConditionalFact(IsMultithreadingSupported) to CircularCctor tests that
require multiple threads. These tests deadlock/crash on single-threaded
WASM where threading is not available.

Affected tests:

  • CircularCctorFourThreads
  • CircularCctorThreeThreads01/02/03
  • CircularCctorThreeThreads01BFI/02BFI/03BFI (IL files)

Copilot AI review requested due to automatic review settings March 25, 2026 20:43
@radekdoulik radekdoulik added this to the 11.0.0 milestone Mar 25, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @agocke, @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents deadlocks/crashes when running CoreCLR loader/type-initialization tests on single-threaded WASM by conditionally skipping the specific test entrypoints that require real multithreading support.

Changes:

  • Switch affected C# test entrypoints from [Fact] to [ConditionalFact(...IsMultithreadingSupported)].
  • Update the corresponding BFI .il test entrypoints to use Xunit.ConditionalFactAttribute with TestLibrary.PlatformDetection.IsMultithreadingSupported.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorFourThreads.cs Skip the multithreaded test on platforms without multithreading support.
src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorThreeThreads01.cs Skip the multithreaded test on platforms without multithreading support.
src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorThreeThreads02.cs Skip the multithreaded test on platforms without multithreading support.
src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorThreeThreads03.cs Skip the multithreaded test on platforms without multithreading support.
src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorThreeThreads01BFI.il Replace FactAttribute with ConditionalFactAttribute gated on IsMultithreadingSupported.
src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorThreeThreads02BFI.il Replace FactAttribute with ConditionalFactAttribute gated on IsMultithreadingSupported.
src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorThreeThreads03BFI.il Replace FactAttribute with ConditionalFactAttribute gated on IsMultithreadingSupported.

@github-actions

This comment has been minimized.

…eaded WASM

Add ConditionalFact(IsMultithreadingSupported) to CircularCctor tests that
require multiple threads. These tests deadlock/crash on single-threaded
WASM where threading is not available.

Affected tests:
- CircularCctorFourThreads
- CircularCctorThreeThreads01/02/03
- CircularCctorFourThreadsBFI (IL file)
- CircularCctorThreeThreads01BFI/02BFI/03BFI (IL files)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@radekdoulik radekdoulik force-pushed the wasm-disable-circularcctor-threading-tests branch from e9fae38 to 3e40f0e Compare March 25, 2026 21:26
@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings March 26, 2026 09:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

@github-actions
Copy link
Contributor

🤖 Copilot Code Review — PR #126120

Holistic Assessment

Motivation: Justified. These CircularCctor tests create 3–4 threads to exercise circular static constructor deadlock detection. On single-threaded WASM, threading APIs throw PlatformNotSupportedException, causing crashes and deadlocks. The sibling test CircularCctorTwoThreadsThreeCC.cs was already updated with this same pattern, confirming this is a known gap.

Approach: Correct. Using [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] is the established pattern in this test directory. It is semantically superior to the previous [ActiveIssue] on Browser (used in FourThreadsBFI.il) because the issue is not a temporary bug — threading is fundamentally unavailable on single-threaded WASM. ConditionalFact cleanly skips the test rather than marking it as a known failure.

Summary: ✅ LGTM. Clean, consistent, and complete change. All multi-threading CircularCctor tests are covered (the only sibling, CircularCctorTwoThreadsThreeCC.cs, was already updated). The IL encoding follows the correct constructor signature. One minor cleanup opportunity noted below, but it is non-blocking.


Detailed Findings

✅ Correctness — ConditionalFact applied correctly

All 4 C# files correctly replace [Fact] with [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))]. The existing [ActiveIssue("needs triage", typeof(PlatformDetection), nameof(PlatformDetection.IsSimulator))] attribute is properly preserved — it covers a separate concern (simulator platforms) unrelated to threading support.

✅ IL encoding — Correct attribute encoding

The IL files correctly encode the ConditionalFactAttribute constructor:

.custom instance void [Microsoft.DotNet.XUnitExtensions]Xunit.ConditionalFactAttribute::.ctor(
    class [System.Runtime]System.Type, string[]) = {
    type([TestLibrary]TestLibrary.PlatformDetection)
    string[1] ('IsMultithreadingSupported')
}

The required assembly externs (Microsoft.DotNet.XUnitExtensions, TestLibrary, System.Runtime) are properly added. Note: this PR uses [System.Runtime]System.Type while some older IL tests (e.g., mcc_interop) use [mscorlib]System.Type — both are valid; System.Runtime is the modern canonical reference.

✅ Completeness — All multi-threaded tests covered

Verified that the CircularCctors/ directory contains exactly 9 test source files. All multi-threading tests are now covered:

  • CircularCctorTwoThreadsThreeCC.cs — already had ConditionalFact (pre-existing)
  • CircularCctorFourThreads.cs + FourThreadsBFI.il — updated ✓
  • CircularCctorThreeThreads01/02/03.cs + 01/02/03BFI.il — updated ✓

✅ TestLibrary dependency — Correctly resolved

The .ilproj files don't explicitly reference TestLibrary, but Directory.Build.targets auto-includes $(TestLibraryProjectPath) via IsMergedTestRunnerAssembly=true (the default). This matches the existing build infrastructure.

✅ ActiveIssue removal in FourThreadsBFI.il — Appropriate

The FourThreadsBFI.il previously had:

.custom instance void [...]Xunit.ActiveIssueAttribute::.ctor(string, valuetype [...]Xunit.TestPlatforms) = {
    string('System.Threading.Thread.ThrowIfMultithreadingIsNotSupported: PlatformNotSupportedException')
    int32(0x400) // TestPlatforms.Browser
}

This was a workaround for the exact same threading issue. Replacing it with ConditionalFact(IsMultithreadingSupported) is strictly better: it covers all single-threaded platforms (not just Browser) and uses a proper skip mechanism rather than marking an expected failure.

💡 Unused xunit.core assembly extern in IL files — Minor cleanup

After removing [Fact] (which was [xunit.core]Xunit.FactAttribute), the .assembly extern xunit.core {} declaration is no longer referenced in any of the 4 IL files. This is harmless dead metadata, but could be cleaned up for tidiness. Non-blocking, fine as a follow-up.

Note

This review was generated by Copilot and reflects an automated analysis of the code changes.

Generated by Code Review for issue #126120 ·

@radekdoulik
Copy link
Member Author

/ba-g stucked infrastructure

@radekdoulik radekdoulik enabled auto-merge (squash) March 26, 2026 15:04
@radekdoulik radekdoulik merged commit 039ef3f into main Mar 26, 2026
81 of 91 checks passed
@radekdoulik radekdoulik deleted the wasm-disable-circularcctor-threading-tests branch March 26, 2026 15:05
@radekdoulik
Copy link
Member Author

ci-analysis, to be sure the runtime tests were not broken

● Let me check if any had actual test failures:

● Check all 74 work items for exit codes (shell)
│ failed_count=0
│ passed_count=0
│ for wi in $(curl -s "https://helix.dot.net/api/jobs/965928c6-fbde-4278-b6a7-4fa007837ec8/workitems?api-version=2019-06-17" | python3 -c "import json,sys; [print(w.get('Name',w.get('name',''))) for w
│ in json.load(sys.stdin)]"); do
│ result=$(curl -sL "https://helix.dot.net/api/jobs/965928c6-fbde-4278-b6a7-4fa007837ec8/workitems/$wi/console?api-version=2019-06-17" 2>/dev/null | grep "Exit Code:" | tail -1)…
└ 13 lines...

● Read shell output Waiting up to 120 seconds for command output
└ 6 lines...

● Shell "Check all 74 work items for exit codes" completed

● Yes — all real tests completed and passed. Here's the summary:

  • 73/74 work items exited with code 0 (tests passed)
  • 3 "non-zero" are false positives: HelixController, Work, Queueing — these are fragments of the single entry HelixController Work Queueing (a controller job, not a test)
  • The AzDO job shows as CANCELLED because it hit the 3h timeout while Helix items were queued (3h26m queue delay!). Tests actually ran in ~1-3 min each.
  • Exit code -4 on work items = failed to upload results back to AzDO (auth token expired after cancellation), not test failures.

Verdict: All coreclr/browser-wasm Pri0 runtime tests passed. The "failure" is purely infrastructure — AzDO timed out waiting for Helix queue, then revoked the upload token.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants