Complete ClusterDaemon and port AutoDown and AutoDownSpec by smalldave · Pull Request #2 · Aaronontheweb/akka.net

smalldave · 2014-09-09T16:48:43Z

No description provided.

Complete ClusterDaemon and port AutoDown and AutoDownSpec

…uild Two follow-ups from the third code-review pass on the akkadotnet#8031 fix: - Distribution (finding #2): the key+1 linear probe placed a relocated colliding virtual node on a near-zero-width ring segment, so a collided node lost ~1/factor of its traffic - the "distribution unchanged" claim was false. Re-hash the loser to a well-distributed slot (full-width segment) instead, preserving the node's ring share, then linear-probe from there to guarantee termination. Non-colliding builds are unchanged (probe never fires); the sequence is a pure function of the node hash so every node still builds an identical ring. - Perf (finding #3): operator +/- passed _nodes.Values (N*virtualNodesFactor entries) to Create, so it sorted/ToString'd N*V items per membership change. Distinct() the same-reference repeats down to N first; Create's ToString de-dup remains the correctness guarantee. Not changed: NullReferenceException on a null ToString() (finding #1) is pre-existing (old Create hashed node.ToString() identically), unreachable from the router (ConsistentRoutee.ToString is never null), and outside the akkadotnet#8031 scope.

… 32-bit hash collision (akkadotnet#8294) * Fix akkadotnet#8031: consistent-hashing router wedges cluster-wide on 32-bit hash collision ConsistentHash.Create now linear-probes to the next free slot when two virtual nodes collide in the 32-bit ring, instead of letting SortedDictionary.Add throw. The throw was swallowed by ConsistentHashingRoutingLogic.Select and returned NoRoutee for every message until a manual restart (and crashed the unguarded ClusterReceptionist). The ring is now built in canonical node order so every node in the cluster produces an identical ring even when a collision is resolved. For any routee set without a collision the ring is byte-identical to prior versions (safe for rolling upgrades) - proven by ConsistentHashSpec. operator + is hardened the same way. Adds ConsistentHashSpec (collision tolerance, distribution-neutrality, cross-node determinism, and a byte-identical before/after proof), a router-level no-wedge test, and a Create scaling benchmark. Perf follow-up: akkadotnet#8293. * Address xhigh review: make ConsistentHash +/- consistent with Create The akkadotnet#8031 fix made Create and operator+ linear-probe past 32-bit collisions, but left operator- computing only natural vnode keys — so it could not remove a vnode that had been relocated to a probed slot, leaving a phantom entry that still routed to the removed node. operator+ also silently duplicated an already-present node's vnodes and resolved collisions in insertion order rather than Create's canonical order (so incremental rings could diverge from Create). Rewrite operator+/- to rebuild deterministically via Create, so `Create(S) + x == Create(S ∪ {x})` and `Create(S) - x == Create(S \ {x})` hold by construction: symmetric, canonical-order collision resolution, idempotent add, and removal that drops probed slots. Drops the now-unused SortedDictionary CopyAndAdd/CopyAndRemove path and duplicated probe loop. Adds regression tests: add==Create-across-collision, idempotent add, and remove-drops-probed-slots. * Address re-review: unify ConsistentHash node identity on ToString() The prior review-fix used EqualityComparer<T>.Default in operator +/- but the ring identifies nodes by ToString() (the value its keys are derived from; the class contract requires ToString to be distinct per node). That mismatch left three confirmed issues: - Create did not de-duplicate, so a node supplied twice was probed into a second vnode set (distribution skew); the dedup guard was only on +/-. - operator+ idempotency relied on Distinct()'s reference equality, so re-adding a fresh-but-equal reference-type node duplicated its vnodes unbounded. - operator- removed by EqualityComparer<T>.Default, so a T whose Equals is broader than ToString could over-remove a different node. Unify identity on ToString(): Create now de-duplicates input by ToString (and +/- inherit it by delegating to Create); operator- matches the removed node by ToString rather than T.Equals. Adds tests for dedup, ToString-based idempotent add, and ToString-based removal using a reference type without an Equals override. * Address 3rd review: full-width collision relocation + cheaper +/- rebuild Two follow-ups from the third code-review pass on the akkadotnet#8031 fix: - Distribution (finding #2): the key+1 linear probe placed a relocated colliding virtual node on a near-zero-width ring segment, so a collided node lost ~1/factor of its traffic - the "distribution unchanged" claim was false. Re-hash the loser to a well-distributed slot (full-width segment) instead, preserving the node's ring share, then linear-probe from there to guarantee termination. Non-colliding builds are unchanged (probe never fires); the sequence is a pure function of the node hash so every node still builds an identical ring. - Perf (finding #3): operator +/- passed _nodes.Values (N*virtualNodesFactor entries) to Create, so it sorted/ToString'd N*V items per membership change. Distinct() the same-reference repeats down to N first; Create's ToString de-dup remains the correctness guarantee. Not changed: NullReferenceException on a null ToString() (finding #1) is pre-existing (old Create hashed node.ToString() identically), unreachable from the router (ConsistentRoutee.ToString is never null), and outside the akkadotnet#8031 scope.

… 32-bit hash collision (akkadotnet#8294) (akkadotnet#8305) * Fix akkadotnet#8031: consistent-hashing router wedges cluster-wide on 32-bit hash collision ConsistentHash.Create now linear-probes to the next free slot when two virtual nodes collide in the 32-bit ring, instead of letting SortedDictionary.Add throw. The throw was swallowed by ConsistentHashingRoutingLogic.Select and returned NoRoutee for every message until a manual restart (and crashed the unguarded ClusterReceptionist). The ring is now built in canonical node order so every node in the cluster produces an identical ring even when a collision is resolved. For any routee set without a collision the ring is byte-identical to prior versions (safe for rolling upgrades) - proven by ConsistentHashSpec. operator + is hardened the same way. Adds ConsistentHashSpec (collision tolerance, distribution-neutrality, cross-node determinism, and a byte-identical before/after proof), a router-level no-wedge test, and a Create scaling benchmark. Perf follow-up: akkadotnet#8293. * Address xhigh review: make ConsistentHash +/- consistent with Create The akkadotnet#8031 fix made Create and operator+ linear-probe past 32-bit collisions, but left operator- computing only natural vnode keys — so it could not remove a vnode that had been relocated to a probed slot, leaving a phantom entry that still routed to the removed node. operator+ also silently duplicated an already-present node's vnodes and resolved collisions in insertion order rather than Create's canonical order (so incremental rings could diverge from Create). Rewrite operator+/- to rebuild deterministically via Create, so `Create(S) + x == Create(S ∪ {x})` and `Create(S) - x == Create(S \ {x})` hold by construction: symmetric, canonical-order collision resolution, idempotent add, and removal that drops probed slots. Drops the now-unused SortedDictionary CopyAndAdd/CopyAndRemove path and duplicated probe loop. Adds regression tests: add==Create-across-collision, idempotent add, and remove-drops-probed-slots. * Address re-review: unify ConsistentHash node identity on ToString() The prior review-fix used EqualityComparer<T>.Default in operator +/- but the ring identifies nodes by ToString() (the value its keys are derived from; the class contract requires ToString to be distinct per node). That mismatch left three confirmed issues: - Create did not de-duplicate, so a node supplied twice was probed into a second vnode set (distribution skew); the dedup guard was only on +/-. - operator+ idempotency relied on Distinct()'s reference equality, so re-adding a fresh-but-equal reference-type node duplicated its vnodes unbounded. - operator- removed by EqualityComparer<T>.Default, so a T whose Equals is broader than ToString could over-remove a different node. Unify identity on ToString(): Create now de-duplicates input by ToString (and +/- inherit it by delegating to Create); operator- matches the removed node by ToString rather than T.Equals. Adds tests for dedup, ToString-based idempotent add, and ToString-based removal using a reference type without an Equals override. * Address 3rd review: full-width collision relocation + cheaper +/- rebuild Two follow-ups from the third code-review pass on the akkadotnet#8031 fix: - Distribution (finding #2): the key+1 linear probe placed a relocated colliding virtual node on a near-zero-width ring segment, so a collided node lost ~1/factor of its traffic - the "distribution unchanged" claim was false. Re-hash the loser to a well-distributed slot (full-width segment) instead, preserving the node's ring share, then linear-probe from there to guarantee termination. Non-colliding builds are unchanged (probe never fires); the sequence is a pure function of the node hash so every node still builds an identical ring. - Perf (finding #3): operator +/- passed _nodes.Values (N*virtualNodesFactor entries) to Create, so it sorted/ToString'd N*V items per membership change. Distinct() the same-reference repeats down to N first; Create's ToString de-dup remains the correctness guarantee. Not changed: NullReferenceException on a null ToString() (finding #1) is pre-existing (old Create hashed node.ToString() identically), unreachable from the router (ConsistentRoutee.ToString is never null), and outside the akkadotnet#8031 scope. (cherry picked from commit cdec84e)

Pure, stage-agnostic InboundCompression: the receiver-side table rotation state machine ported from Pekko's InboundCompression[T] + Tables[T]. - active/next/oldTables (capped at KeepOldTables=3, starts disabled@0xFF) + advertisementInProgress; string-keyed, single class serves refs + manifests. - Decompress: selectTable(active -> old -> in-progress flip+retry); disabled/ unknown/greater version and out-of-range index -> clean miss, never throws. - ConfirmAdvertisement: Ack (trigger #1) and give-up both flip via startUsingNextTable; mismatched/absent in-progress -> no-op. - Decompress at the in-progress version flips without an Ack (trigger #2). - BuildNextAdvertisement: build from heavy hitters at IncrementVersion(active) (== nextTable.version), stash invert as next, resend up to maxResendCount=3, then give up and flip anyway. version wraps 0..127, 127 -> 0. - Hit feeds the Stage 1 sketch + TopHeavyHitters; excludes null/empty. No locks (single-threaded stage ownership), no timer/actor/GraphStage/outbound send -- those are Stage 2b-ii. ArteryInboundProcessingStage untouched.

… tests 13 pure unit tests for InboundCompression (net10.0, 27ms, no actor system): - confirm via Ack: active updated, previous retained in old, indices resolve - confirm via first-stamped message (trigger #2): Decompress flips without Ack - KeepOldTables=3: after 5 rotations exactly [4,3,2] kept; in-window resolves, older-than-window -> miss - version wrap: 0..127 then 127 -> 0, never exceeds 127 - unknown/greater version -> miss, no throw; out-of-range index -> miss, no throw - advertisementInProgress: resend x3 (same table) then give up and flip anyway; fresh build afterward advertises the incremented version - disabled/empty state resolves nothing; Hit excludes null/empty/non-positive - advertised version == IncrementVersion(active) across rotations Artery suite: 229 prior + 13 new = 242, all green.

…default, correctness edges) Two-system integration (ArteryCompressionRoundTripSpec): - Round trip: A->B repeated sends; asserts A installs the table B advertised and B publishes a Resolved event (a COMPRESSED tag crossed the wire and decoded) with correct end-to-end delivery throughout. - Off-by-default: compression off => no advertisement, no table, no compression event, LITERAL wire, byte-identical; delivery unaffected. Coordinator-level, deterministic (InboundCompressionsImplSpec) against a fake IInboundCompressionContext capturing sends + events: - observe -> advertise (dense entries + Advertised event), - unchanged-table `alive` gate (no re-advertise until a new hit), - Ack-loss -> trigger #2 (first stamped message activates + resolves), - restarted-incarnation (unknown/greater version -> miss, never throws, then a fresh table is re-advertised), - resend-then-give-up lifecycle, - close-on-no-association, and independent manifest advertisement. Full Artery suite green: 242 prior + 9 new = 251. Akka.API.Tests unchanged.

…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.

Complete ClusterDaemon and port AutoDown and AutoDownSpec

01f9ba3

Aaronontheweb added a commit that referenced this pull request Sep 9, 2014

Merge pull request #2 from smalldave/akka-cluster

85e2aac

Complete ClusterDaemon and port AutoDown and AutoDownSpec

Aaronontheweb merged commit 85e2aac into Aaronontheweb:akka-cluster Sep 9, 2014

Navigation Menu

Complete ClusterDaemon and port AutoDown and AutoDownSpec - #2

Complete ClusterDaemon and port AutoDown and AutoDownSpec#2
Aaronontheweb merged 1 commit into
Aaronontheweb:akka-clusterfrom
smalldave:akka-cluster

smalldave commented Sep 9, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

smalldave commented Sep 9, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants