fix(transit/delta-message): clone JsonObject/JsonArray on receive (fixes #241)#321
Merged
Conversation
#241) DeltaMessageTransit<T>.FromJsonNode used node.AsObject() / node.AsArray() for the JsonObject and JsonArray branches. AsObject and AsArray are downcasts that return the same instance — so when ReceiveAsync was followed by a delta message, FromJsonNode handed the caller a live reference to _lastReceivedState. Caller mutation silently contaminated the receiver's internal state, and any subsequent delta produced a desynced result. Concurrent enumeration during a receive-pump-applied delta also threw 'Collection was modified'. The JsonNode branch already DeepClones; this fix makes JsonObject and JsonArray consistent: DeepClone first, then AsObject/AsArray. JsonDocument and JsonElement branches already produce fresh instances via JsonDocument.Parse, and the typed-deserializer branch returns a fresh deserialization. Adds tests/NetConduit.Transit.DeltaMessage.UnitTests/DeltaMessageTransitJsonAliasingTests.cs with 3 tests: JsonObject mutation isolation across deltas, JsonObject distinct references across consecutive delta receives, and JsonArray mutation isolation. All three fail on master with the exact symptoms from issue #241 ('extra' key bleeds through, ReferenceEquals true, ghost array element); all three pass with the fix.
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
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.
Fixes #241
Problem
DeltaMessageTransit<T>.FromJsonNodehad inconsistent aliasing semantics acrossTchoices:TJsonNodenode.DeepClone()JsonObjectnode.AsObject()_lastReceivedState❌JsonArraynode.AsArray()_lastReceivedState❌JsonDocumentJsonDocument.Parse(...)JsonElementJsonDocument.Parse(...).RootElementnode.Deserialize(_typeInfo)AsObject()/AsArray()are downcasts, not copies. WhenReceiveAsyncreturned via the delta path (case 0x01),FromJsonNode(_lastReceivedState)handed the caller the sameJsonObject/JsonArrayinstance the receive pump mutates on every subsequent delta.Two consequences:
JsonObjectenumerator throwsInvalidOperationException("Collection was modified ...").The full-state branch (
case 0x00) was already safe because it stores_lastReceivedState = fullState?.DeepClone()and returnsFromJsonNode(fullState)wherefullStateis the freshly-parsed wire node. The bug is delta-path-specific.Fix
Make
JsonObject/JsonArrayconsistent with theJsonNodebranch —DeepClonefirst, then downcast:The clone is unconditional but cheap relative to the existing JSON parse on the receive path; full-state already clones once into
_lastReceivedState, so this just adds a second clone for the returned value (which is the standard "snapshot" contract for a thread-safe receive API).Validation
tests/NetConduit.Transit.DeltaMessage.UnitTests/DeltaMessageTransitJsonAliasingTests.cs— 3 tests:JsonObject_DeltaResult_MutationDoesNotContaminateInternalState— full state → delta receive → mutate result → next delta. With bug: third receive carries the injectedextrakey. With fix: clean.JsonObject_DeltaResult_IsNotAliasedAcrossSubsequentReceives— full state → delta → delta. With bug: second and third results are the same reference. With fix: distinct.JsonArray_DeltaResult_MutationDoesNotContaminateInternalState— same shape forJsonArray. With bug: ghost element 999 leaks into next delta result.Files changed
src/NetConduit.Transit.DeltaMessage/DeltaMessageTransit.csDeepClone()beforeAsObject()/AsArray()inFromJsonNodetests/NetConduit.Transit.DeltaMessage.UnitTests/DeltaMessageTransitJsonAliasingTests.csJsonObject+JsonArray, delta path, mutation isolation, reference distinctness)Out of scope
The issue's bonus note about
JsonElement/JsonDocumentlifetime (theJsonDocumentreturned fromJsonDocument.Parse(...).RootElementis anchored only via theJsonElement, leaving pooled buffers reclaimed via finalization) is a separate concern and is not addressed in this PR — it's a different shape of bug and a different fix surface. Tracked separately if it warrants a follow-up.