Backport v1.5.71: ClusterClientDiscovery fix, PromiseActorRef torn read, Remote throttler, TestKit AutoDilate - #8466
Merged
Conversation
…ss rediscovery (akkadotnet#8426) * Fix ClusterClientDiscovery: preserve contact-point subscriptions across rediscovery ClusterClientDiscovery supervises the real ClusterClient as a child and recreates that child from scratch on every rediscovery (when the current child exhausts its reconnect-timeout and stops). The contact-point subscriber list lives on the child, so a SubscribeContactPoints subscriber silently stopped receiving ContactPoints/ContactPointAdded/ContactPointRemoved once the client rediscovered — the fresh child had no record of it. Track contact-point subscribers at the supervisor level and re-subscribe them onto each newly-created child (on the subscriber's behalf, so the child registers them and replies with the current snapshot). DeathWatch each subscriber so a terminated subscriber is dropped from tracking (auto-unsubscribe). Subscribe/Unsubscribe are still forwarded to the current child, so live behavior is unchanged; only the across-rediscovery gap is closed. * ClusterClientDiscoverySpec: verify contact points via subscription events Replace the three GetContactPoints polling blocks (each with an orphaned, hand-tightened 1s inner ExpectMsg that spuriously timed out on loaded CI agents) with a single event-driven helper: subscribe via SubscribeContactPoints and wait on the client's own ContactPoints/ContactPointAdded/ContactPointRemoved stream until the contact points settle to exactly the expected node. No per-attempt reply timeout to race. This also covers the ClusterClientDiscovery subscription-survival fix: the second and third phases exercise rediscovery after a graceful down and a hard shutdown, so the subscription must survive the child being recreated for the helper to observe the new node. Verified locally, 4/4 across repeated runs.
…ol>, ...) (akkadotnet#8430) This overload resolved its bound as RemainingOrDilated(RemainingOrDilated(timeout)), applying akka.test.timefactor twice. Every sibling overload applies it once. With timefactor = 3 an explicit 5s wait became 45s. The null case is worse: the inner call resolves to RemainingOrDefault - the time actually left in the enclosing Within - and the outer call then dilates that, producing a bound larger than the remaining budget it was derived from. Apply RemainingOrDilated once, matching every other overload. No in-repo caller uses this overload, so nothing here was relying on the inflated bound; Akka.TestKit.Tests (320) and Akka.TestKit.Xunit2.Tests (4) pass.
…kkadotnet#8436) * Fix torn read in PromiseActorRef.GetPath that made Path return null `PromiseActorRef.GetPath()` matched on its `State` field and then re-read that same field to build the return value: switch (State) // read #1 - matches ActorPath { ... case ActorPath _: return State as ActorPath; // read #2 - re-reads the field `State` is an `AtomicReference<object>` whose getter is a `Volatile.Read`, so those are two genuinely separate loads of a field any thread can mutate. A concurrent `Stop()` flipping the state from `ActorPath` to `StoppedWithPath` between the two reads makes the `as` cast fail, and `Path` returns **null** - violating the method's own contract ("Must always return the same ActorPath"). The sibling `case StoppedWithPath stoppedWithPath:` was already correct because it binds the pattern variable instead of re-reading. A null path then dereferences twice, unguarded, in `ActorRefBase.GetHashCode()` and blows up. Observed in CI: System.NullReferenceException at Akka.Actor.ActorRefBase.GetHashCode() at System.Collections.Generic.HashSet`1.Remove(T item) at Akka.Actor.FullActorState.RemoveWatchedBy(IActorRef) at Akka.Actor.ActorCell.RemWatcher(...) at Akka.Actor.ActorCell.SysMsgInvokeAll(...) The interleaving comes from `AskModeWithDeathCompletion` in ThrottleTransportAdapter, which does Watch -> Tell -> SendSystemMessage(Unwatch) while the promise completes and stops on another thread. Throwing out of `SysMsgInvokeAll` kills an arbitrary actor mid-Unwatch; in the observed case it restarted a transport throttler into a permanently broken state. Fix, in two parts: * `GetPath()` now snapshots `State` into a local at the top of each loop iteration and switches on the local, binding the pattern variable (`case ActorPath path: return path;`). The snapshot is per-iteration, not hoisted out of the loop, so the `Stopped`/`Registering`/`null` cases still re-read after the state transition they wait on. All other case semantics are unchanged. * `ActorRefBase.ToString/GetHashCode/Equals/CompareTo` cache `Path` (and the other ref's `Path`) into locals instead of dereferencing the virtual property two to four times per call. No null guard is added: after the `GetPath()` fix, every return path is non-null - a failing `Provider.TempPath()` throws rather than returning null - so null is impossible at the source and swallowing it here would only hide the next such bug. The local cache is still correct practice: it removes the torn-read window for any `IActorRef` whose `Path` is computed from mutable state, and drops redundant virtual calls (`GetPath()` is a CAS loop, not a field read). Regression test: `PromiseActorRefSpec` pins one `PromiseActorRef` between a reader thread spinning on `GetHashCode`/`Path` and a stopper thread that waits until the reader is demonstrably inside that spin before calling `Stop()`, so the state flip always lands with a read in flight. Against the unfixed code it reproduced 5/5 runs, 100-240 null paths and 90-290 NREs per 2,000 rounds, with the failure stack matching the CI report. It passes 8/8 runs after the fix and takes ~250ms. Akka.Tests: 1306 passed, 0 failed, 23 skipped. Akka.Remote.Tests: 645 passed, 0 failed, 5 skipped. Akka.API.Tests: 18 passed - no public API surface change. * Narrow to the actual fix: drop the ActorRef caching and the race-repro spec Two things did not earn their place in this change. The ActorRef.cs Path caching was presented as protection against a torn read, but it protects against nothing: PromiseActorRef.Stop() transitions ActorPath -> StoppedWithPath(p) carrying the SAME path instance (Futures.cs:608), so two consecutive Path reads cannot disagree. Reverting Futures.cs while keeping the caching also still reproduced the NRE, proving the caching was never the fix. What remained was a micro-optimization with a misleading safety comment, widening a change that should be trivially reviewable and backportable. PromiseActorRefSpec reproduced the race by spinning thousands of rounds hoping to hit an interleaving. It did its job proving the fix during development, but shipping a probabilistic, CPU-heavy timing test into the permanent suite adds exactly the kind of flake this suite is being cleaned of. What is left is the one defect: GetPath() matched on State and then re-read the field, so a concurrent Stop() made 'State as ActorPath' return null.
…et#8433, akkadotnet#8434) (akkadotnet#8437) ThrottlerManager never managed the lifecycle of its ThrottledAssociation children, which produced two related failures. 1. No SupervisorStrategy was declared, so the default Restart applied. A restarted inbound ThrottledAssociation re-enters WaitExposedHandle, and the Handle message that leaves that state is only ever sent once - at creation. The association then sat there for the life of the process, logging "unhandled event InboundPayload" and dropping every inbound packet, with no disassociation and no quarantine to tell remoting anything was wrong. ThrottlerManager now stops failed children, matching AkkaProtocolManager. 2. _handleTable entries were only removed on the ForceDisassociate paths, so a throttler that died any other way left a stale entry behind. Later SetThrottle/PassThrough walks messaged dead refs - the throttle mode dead-lettered and the change silently did not apply. Children are now watched at their single creation site and purged from the table on Terminated. Belt and braces: an InboundPayload arriving in WaitExposedHandle is impossible during a healthy startup - the read handler is not registered until we leave that state - so it now disassociates and stops instead of dropping packets quietly. Scope: the throttle adapter is only in the transport pipeline when applied-adapters includes trttl/gremlin, i.e. multi-node test runs rather than production deployments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of 5 merged dev PRs to v1.5 for the v1.5.71 maintenance release.
Changes
ExpectMsgAsync<T>(Func<T, IActorRef, bool>, ...)PromiseActorRef.GetPaththat madePathreturn nullAPI approval
Includes the
AutoDilateAttributeadditions toCoreAPISpec.ApproveTestKit.DotNet/Net.verified.txt(new public type from #8441). Verified locally: all 15 net10.0 approval tests pass.Notes