[xUnit 3] Fix parallel-class implicit-sender leak in Akka.TestKit.Xunit#8174
[xUnit 3] Fix parallel-class implicit-sender leak in Akka.TestKit.Xunit#8174Arkatufus wants to merge 10 commits into
Conversation
Under xUnit v3 parallel-class scheduling, MaxConcurrencySyncContext dispatches test ctors and bodies onto a dedicated pool of reused threads. TestKitBase's constructor pins InternalCurrentActorCellKeeper.Current (a ThreadStatic) on its ctor thread; when a sibling test's body lands on that same reused thread, the pre-await synchronous prefix of the [Fact] reads the sibling's cell as its implicit sender, causing Tell() to use the wrong Sender. Replies then cross ActorSystem boundaries. Fix: add a BeforeAfterTestAttribute that runs synchronously on the test- body thread and (a) pins Current to the running test's TestActor cell, (b) installs ActorCellKeepingSynchronizationContext so await continuations also re-pin the cell. After the test, Current is cleared so the reused worker doesn't carry the cell into the next test. Applied to Akka.TestKit.Xunit.TestKit so derived test classes (including Akka.Hosting.TestKit downstream) get parallel-safe behavior automatically via attribute inheritance. ActorCellKeepingSynchronizationContext promoted to [InternalApi] public so the attribute can install an equivalent wrapper without duplicating the save/pin/restore logic. TBD XML docs replaced with real documentation. Regression tests: - ParallelAmbientContextSpec: 16 + 8 sibling test classes asserting implicit-sender correctness and INoImplicitSender == null across awaits. Marked [LocalFact] — requires xUnit.ParallelizeTestCollections=true to exercise the bug (disabled in the shared src/xunit.runner.json). - AkkaCleanAmbientContextAttributeSpec: reflection guards that the attribute is declared on the base TestKit, is inherited by subclasses, and has Inherited=true in its AttributeUsage.
| /// mechanism and the ThreadStatic-vs-ExecutionContext rationale. | ||
| /// </summary> | ||
| [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] | ||
| public sealed class AkkaCleanAmbientContextAttribute : BeforeAfterTestAttribute |
There was a problem hiding this comment.
The fix. Since xUnit 3 can switch thread executor/context between the constructor, InitializeAsync() invocation, and the actual test method invocation, we pin the test actor into the ActorCellKeepingSynchronizationContext right before the test starts and clear it after when it is being teared down.
BeforeAfterTestAttribute is a new attribute in xUnit 3 for test setup and tear down.
| /// This class represents an Akka.NET TestKit that uses <a href="https://xunit.github.io/">xUnit</a> | ||
| /// as its testing framework. | ||
| /// </summary> | ||
| [AkkaCleanAmbientContext] |
There was a problem hiding this comment.
Mark TestKit with the new AkkaCleanAmbientContext. This attribute is marked as Inherited so all classes that inherits TestKit will automatically have this attribute.
…Arkatufus/akka.net into xunit-3-implicit-sender-leak-D2
Keep ActorCellKeepingSynchronizationContext internal via friend assembly access and run Akka.TestKit.Xunit.Tests with parallel collections so CI exercises the implicit-sender leak regression by default.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Generally looks good but have some nitpicks
| @@ -1,6 +1,4 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <Import Project="../../../xunitSettings.props" /> | |||
There was a problem hiding this comment.
Going to use a unique, project-specific xunit settings file for this so we can enable test parallelism and see about reproducing issues like akkadotnet/Akka.Hosting#733 in CI/CD.
| // Forces the post-ctor continuation onto a different SC worker — the | ||
| // thread pollution only manifests when the body thread differs from the | ||
| // ctor thread. | ||
| public async ValueTask InitializeAsync() => await Task.Yield(); |
| public ValueTask DisposeAsync() => default; | ||
|
|
||
| [Fact] | ||
| public async Task Implicit_sender_should_resolve_to_own_TestActor() |
|
|
||
| // The following GUID is for the ID of the typelib if this project is exposed to COM | ||
| [assembly: Guid("ae01b790-1478-4917-9299-b4855ba997cb")] | ||
| [assembly: InternalsVisibleTo("Akka.TestKit.Xunit")] |
There was a problem hiding this comment.
Make Akka.TestKit.Xunit a friend assembly - that's preferable to exposing more things on the public API.
There was a problem hiding this comment.
These are just TBD cleanup, which is appreciated.
| { | ||
| "$schema": "https://xunit.github.io/schema/current/xunit.runner.schema.json", | ||
| "longRunningTestSeconds": 60, | ||
| "parallelizeAssembly": true, |
There was a problem hiding this comment.
Ensures we run the Akka.TestKit.Xunit.Tests assembly in parallel to try to get some CI/CD coverage over the test isolation guarantees.
Stop swallowing NullReferenceException when reading TestActor and drop the reflection-only attribute spec in favor of the parallel behavior tests that now run in CI.
Aaronontheweb
left a comment
There was a problem hiding this comment.
LGTM - addressed my requested changes from the previous review.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Looks like this approach may not work at all for the Hosting.TestKit, so I'm investigating that separately.
Related: Wrapping SynchronizationContext for Hosting CompatibilityPR #8182 builds on this PR's The wrapping approach:
Proven in Akka.Hosting PR akkadotnet/Akka.Hosting#735 — 303/303 tests pass under real xUnit v3 parallel class execution. See issue akkadotnet/Akka.Hosting#733 for the original bug report. |
|
superseded via #8182 |
Problem
Under xUnit v3 parallel-class scheduling,
MaxConcurrencySyncContextdispatches test ctors and bodies onto a dedicated pool of reused threads.TestKitBase's constructor pinsInternalCurrentActorCellKeeper.Current— a[ThreadStatic]— on its ctor thread. When a sibling test's body lands on that same reused thread, the pre-await synchronous prefix of the[Fact]reads the sibling's cell as its implicit sender, causingTell()to use the wrongSender. Replies then crossActorSystemboundaries and land in the wrongTestActor's mailbox.Originally reported in https://github.com/akkadotnet/Akka.Hosting by a user running tests with
parallelizeTestCollections: true.Fix
Add a
[BeforeAfterTestAttribute]that runs synchronously on the test-body thread and:InternalCurrentActorCellKeeper.Currentto the running test'sTestActorcell (ornullforINoImplicitSender), and installsActorCellKeepingSynchronizationContextsoawaitcontinuations re-pin the cell across worker-thread switches.Currentso the reused worker doesn't carry the cell into the next test.Applied to
Akka.TestKit.Xunit.TestKitwithInherited = true, so derived test classes (includingAkka.Hosting.TestKitdownstream) get parallel-safe behavior automatically via attribute inheritance.Changes
Akka.TestKit/ActorCellKeepingSynchronizationContext.cs— promoted from internal to[InternalApi] publicso the new attribute can install an equivalent wrapper without duplicating the save/pin/restore logic. AllTBDXML docs replaced with real documentation.Akka.TestKit.Xunit/Attributes/AkkaCleanAmbientContextAttribute.cs— newBeforeAfterTestAttribute. UsesTestContext.Current.TestClassInstancefor direct instance access (xUnit v3 only).Akka.TestKit.Xunit/TestKit.cs— decorated with[AkkaCleanAmbientContext].Regression tests
ParallelAmbientContextSpec— 16 + 8 sibling test classes (distinct top-level classes so xUnit schedules them in separate collections). Verified: fails 15/16 without the attribute, passes all with it. Marked[LocalFact]because the sharedsrc/xunit.runner.jsondisables parallelization — exercising the bug requires running withxUnit.ParallelizeTestCollections=true:AkkaCleanAmbientContextAttributeSpec— reflection guards (always run) that the attribute is declared onTestKit, is inherited by derived classes, and hasAttributeUsage.Inherited == true.Scope
Akka.TestKit.Xunit).Akka.TestKit.Xunit2is untouched — the bug is specific to xUnit v3'sMaxConcurrencySyncContextthread-reuse semantics.Akka.TestKit.ActorCellKeepingSynchronizationContext— only visibility + docs.Akka.dll).Validation
Akka.TestKit.Xunit.Tests: 31/31 pass (all three frameworks).