Repository navigation
fix: implement IAsyncLifetime on Akka.TestKit.Xunit (v3) to stop ActorSystem leaks (#8191) - #8217
Aaronontheweb wants to merge 3 commits into
Conversation
…adotnet#8191) xUnit v3 disposes a test class instance via IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever the type implements both (Xunit.IAsyncLifetime derives from IAsyncDisposable). Once a derived spec implemented IAsyncLifetime, the v3 Akka.TestKit.Xunit.TestKit's Dispose() was never invoked, so AfterAll()/Shutdown() never ran and the ActorSystem was silently leaked. TestKit now implements IAsyncLifetime with public virtual InitializeAsync() and DisposeAsync(). DisposeAsync() drives the existing synchronous dispose chain, so a derived override can chain to base.DisposeAsync(). Converts the in-repo specs that hit the trap (Bugfix8144Spec, ParallelAmbientContext*, EventFilterTestBase) to the override form, and adds Bugfix8191Spec as a regression test. The EventFilterTestBase conversion also restores its AfterAll() -> EnsureNoMoreLoggedMessages() verification, which was being skipped.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Looks ok - let me see if we can get rid of some sync-over-async behavior while we're at it.
| /// </summary> | ||
| [AkkaCleanAmbientContext] | ||
| public class TestKit : TestKitBase, IDisposable | ||
| public class TestKit : TestKitBase, IDisposable, IAsyncLifetime |
There was a problem hiding this comment.
this is the primary change here - ensure that the TestKit comports with xUnit3's IAsyncLifetime.
| /// implementation does nothing. Override to perform asynchronous test setup, and | ||
| /// call <c>await base.InitializeAsync()</c> from your override. | ||
| /// </summary> | ||
| public virtual ValueTask InitializeAsync() |
There was a problem hiding this comment.
so we have to do some sync-over-async stuff in the Initialize / Dispose methods in the TestKit today (such as terminating the ActorSystem) - if we could make that natively async that would be great, but I suspect that might be too destructive of a change to make backwards compatible. Might just be a matter of re-arranging the base class calls though. Let me look into that.
There was a problem hiding this comment.
Done in c142966. Added TestKitBase.ShutdownAsync — an async counterpart to Shutdown() that awaits ActorSystem termination via Task.WhenAny(Terminate(), Task.Delay(timeout)) instead of Terminate().Wait(timeout). It's purely additive; the synchronous Shutdown() is untouched.
DisposeAsync() now runs the synchronous dispose chain — Dispose(bool), and therefore AfterAll() plus any derived Dispose(bool) override — with the blocking Shutdown() call suppressed via a flag, then awaits ShutdownAsync(). So the dispose-pattern extensibility point still fires (no silent bypass of a Dispose(bool) override) and AfterAll() still runs while the system is alive, but the actual ActorSystem teardown no longer blocks the xUnit teardown thread.
The constructor side is still synchronous — the ActorSystem is created in the ctor, which can't be made async — so there's no InitializeAsync sync-over-async to remove there; the base InitializeAsync() is just an empty hook.
Heads-up: this touches TestKitBase, so the ApproveTestKit API baselines (.DotNet + .Net) are regenerated in the same commit for the two new ShutdownAsync overloads.
BugFixSpec and StreamRefsSpec consume the xUnit v3 Akka.TestKit.Xunit transitively via the shared xunit3 AkkaSpec. Now that TestKit declares public virtual InitializeAsync()/DisposeAsync(), their own implicit implementations hide the inherited members (CS0114), which the projects' TreatWarningsAsErrors promotes to a build error. Converts both to the override form, dropping the redundant IAsyncLifetime declaration and chaining base.InitializeAsync(). Removing the no-op DisposeAsync overrides also lets the base dispose chain run, fixing the ActorSystem leak these specs had under xUnit v3.
Addresses PR review feedback about sync-over-async teardown. Adds TestKitBase.ShutdownAsync — an async counterpart to Shutdown() that awaits ActorSystem termination instead of blocking on Terminate().Wait(). This is purely additive (extend-only); the existing synchronous Shutdown() is unchanged. Akka.TestKit.Xunit.TestKit.DisposeAsync() now runs the synchronous dispose chain (Dispose(bool) -> AfterAll, plus any derived Dispose(bool) override) with the blocking Shutdown() suppressed via a flag, then awaits ShutdownAsync(). This keeps the dispose-pattern extensibility point intact and terminates the ActorSystem without blocking the xUnit teardown thread. Regenerates the ApproveTestKit API approval baselines for the two new ShutdownAsync overloads.
|
Pushed two follow-up commits:
Verified locally: full solution build clean (0 |
…ync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing.
…ync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing.
…ync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing.
…ync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing.
…ync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing.
…ync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing.
…Async no longer leaks the ActorSystem; de-flake StreamRefsSpec (#8545) * TestKit.Xunit: implement the async dispose chain so derived DisposeAsync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing. * StreamRefsSpec, BugFixSpec, Bugfix8144Spec: migrate to the async TestKit API Convert ExpectMsg, ExpectNoMsg, and .Wait()-on-stream-completion calls to ExpectMsgAsync, ExpectNoMsgAsync, and awaited AwaitWithTimeout across all facts in the three touched files. * StreamRefsSpec, BugFixSpec: finish the async migration; restore INFO logging * ClusterShardingLeaseSpec: override the TestKit's async lifecycle instead of declaring its own * TestKit.Xunit: one disposed flag; Shutdown moves out of Dispose(bool) so the async path needs no mode switch Dispose(bool) now only runs AfterAll() (and any derived teardown); it no longer terminates the ActorSystem and no longer guards re-entrancy. The guard collapses to a single _disposed flag checked once, in each public entry point: Dispose() runs the chain then calls the blocking Shutdown(), DisposeAsync() runs the chain then awaits ShutdownAsync(). This drops the now-redundant _disposing/_disposingAsync flags and the nested try/finally that used to suppress Shutdown() inside the async path. Verified: no Dispose(bool) override in the tree depends on the ActorSystem being torn down when base.Dispose(disposing) returns (the only overrides in the repo are on System.IO.Stream/GraphStageLogic, unrelated to this TestKit); StreamRefsSpec, BugFixSpec's DisposeAsync overrides still tear down their own systems before chaining to base.DisposeAsync(), and ClusterShardingLeaseSpec no longer declares one at all. Added a Bugfix8191Spec case covering Dispose() called after DisposeAsync().
…Async no longer leaks the ActorSystem; de-flake StreamRefsSpec (#8545) * TestKit.Xunit: implement the async dispose chain so derived DisposeAsync no longer leaks the ActorSystem; de-flake StreamRefsSpec Fixes #8191: xUnit v3 calls IAsyncDisposable.DisposeAsync() in preference to IDisposable.Dispose() whenever a type implements both, so a derived spec's own no-op DisposeAsync (e.g. StreamRefsSpec) skipped TestKit's whole synchronous dispose chain and silently leaked a remoting-enabled ActorSystem per test. Akka.TestKit.Xunit.TestKit now implements IAsyncLifetime with a virtual InitializeAsync/DisposeAsync pair; DisposeAsync runs the sync chain and then shuts the system down with the non-blocking ShutdownAsync(). Ports the TestKit.cs shape from stalled PR #8217 (targeted v1.5) onto dev; its TestKitBase.cs hunk is dropped because ShutdownAsync already landed on dev via 81289e8, and EventFilterTestBase.cs is hand-merged onto dev's current AwaitAssertAsync retry body. Five other TestKit-derived specs that declared their own DisposeAsync/InitializeAsync are updated to override and chain to base: BugFixSpec, Bugfix8144Spec (Xunit v3), ParallelAmbientContextSpec (both base classes), and EventFilterTestBase. StreamRefsSpec.SinkRef_must_receive_elements_via_remoting is de-flaked on top of the leak fix: the test now gates on WatchTermination(Keep.Right) instead of asserting on a flat 3s wall-clock wait, uses async TestKit calls throughout, and gives every cross-boundary wait a dilated 15s budget. Remote system port is now 0 and loglevel is DEBUG for stage-level tracing. * StreamRefsSpec, BugFixSpec, Bugfix8144Spec: migrate to the async TestKit API Convert ExpectMsg, ExpectNoMsg, and .Wait()-on-stream-completion calls to ExpectMsgAsync, ExpectNoMsgAsync, and awaited AwaitWithTimeout across all facts in the three touched files. * StreamRefsSpec, BugFixSpec: finish the async migration; restore INFO logging * ClusterShardingLeaseSpec: override the TestKit's async lifecycle instead of declaring its own * TestKit.Xunit: one disposed flag; Shutdown moves out of Dispose(bool) so the async path needs no mode switch Dispose(bool) now only runs AfterAll() (and any derived teardown); it no longer terminates the ActorSystem and no longer guards re-entrancy. The guard collapses to a single _disposed flag checked once, in each public entry point: Dispose() runs the chain then calls the blocking Shutdown(), DisposeAsync() runs the chain then awaits ShutdownAsync(). This drops the now-redundant _disposing/_disposingAsync flags and the nested try/finally that used to suppress Shutdown() inside the async path. Verified: no Dispose(bool) override in the tree depends on the ActorSystem being torn down when base.Dispose(disposing) returns (the only overrides in the repo are on System.IO.Stream/GraphStageLogic, unrelated to this TestKit); StreamRefsSpec, BugFixSpec's DisposeAsync overrides still tear down their own systems before chaining to base.DisposeAsync(), and ClusterShardingLeaseSpec no longer declares one at all. Added a Bugfix8191Spec case covering Dispose() called after DisposeAsync(). (cherry picked from commit 450bebf) v1.5 backport note: re-added .WithFallback(ConfigurationFactory.Load()) to StreamRefsSpec's Config(), which the hand-merge of the port/hostname change had dropped. Akka.Streams.Tests still targets net48 on v1.5 (dev has no netfx lane), and its app.config sets stream.materializer.debug.fuzzing-mode = on for that lane. ActorSystem.Create only calls ConfigurationFactory.Load() when no config is supplied, so without this fallback an explicitly configured ActorSystem - like the ones this spec creates - never saw app.config's fuzzing setting, silently losing that coverage on the net48 lane.
Problem
Fixes #8191.
Xunit.IAsyncLifetime(xunit.v3) derives fromIAsyncDisposable, and xUnit v3 tears a test class instance down viaIAsyncDisposable.DisposeAsync()in preference toIDisposable.Dispose()whenever the type implements both (confirmed inTestRunner<TContext,TTest>.DisposeTestClassInstanceandDisposalTracker).The v3
Akka.TestKit.Xunit.TestKitimplemented onlyIDisposable. As soon as a derived spec addedIAsyncLifetimeitself, xUnit called the derivedDisposeAsync()and skippedTestKit.Dispose()entirely — soAfterAll()andShutdown()never ran and theActorSystemwas silently leaked. There was also nobase.DisposeAsync()to chain to.Several in-repo specs were hitting this and leaking on every run:
Bugfix8144Spec,ParallelAmbientContextSpec*(×24), andEventFilterTestBase— the last of which also meant itsAfterAll()→EnsureNoMoreLoggedMessages()verification was being skipped.Fix
Akka.TestKit.Xunit.TestKitnow implementsIAsyncLifetimewithpublic virtual ValueTask InitializeAsync()andpublic virtual ValueTask DisposeAsync().DisposeAsync()drives the existingDispose(true)→AfterAll()→Shutdown()chain, so a derived override can safelyawait base.DisposeAsync().overrideform (dropping the redundant, IAsyncLifetime). This restoresEventFilterTestBase's post-test verification.Bugfix8191Specas a regression test exercising the exact((IAsyncDisposable)instance).DisposeAsync()path xUnit v3 uses.Compatibility
public virtualmembers on a non-sealed type). Existing compiled consumers keep working;DisposeAsync()runs the same chainDispose()did.CS0114("hides inherited member") only for a consumer that subclasses the v3Akka.TestKit.Xunit.TestKit, addedIAsyncLifetimethemselves, and didn't useoverride/new. That is a warning (build still succeeds) unless they compile withTreatWarningsAsErrors. Remediation is mechanical: addoverride, drop the redundant, IAsyncLifetime. That set of consumers is exactly the set leakingActorSystems today.Akka.TestKit.Xunit2) is untouched and unaffected.Testing
All 10 in-repo projects that consume the v3
Akka.TestKit.Xunitbuild clean with-warnaserror(noCS0114).Akka.TestKit.Xunit.Tests33/33,Akka.TestKit.Tests320/320,Akka.Discovery.Tests27/27.