From c66c1221e492622a4b204ee2b7a5d635f3053abf Mon Sep 17 00:00:00 2001 From: Jose Date: Thu, 23 Apr 2026 10:52:20 +0200 Subject: [PATCH 1/2] Copy source dictionary in RequestContextFeature constructor The outgoing request context is encoded lazily, when the request is sent. If the feature shared the caller's dictionary reference, a concurrent mutation of that dictionary (e.g. a loop that reuses the same dictionary across requests) could change what gets written on the wire. Have the constructor take a snapshot, and document the copy-on-write pattern for interceptors that need to add or remove context entries. Fixes #4454 --- .../RequestContextInterceptorExamples.cs | 24 +++++++++++++++++++ src/IceRpc/Features/IRequestContextFeature.cs | 9 +++++++ src/IceRpc/Features/RequestContextFeature.cs | 8 ++++--- .../RequestContextInterceptorTests.cs | 13 ++++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs b/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs index 48262e7bd9..68ff18ac3a 100644 --- a/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs +++ b/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs @@ -48,4 +48,28 @@ public static async Task UseRequestContext() #endregion } } + + public static void UpdateRequestContextInInterceptor() + { + #region UpdateRequestContextInInterceptor + // An interceptor that adds an entry to the request context. The appropriate pattern is + // copy-on-write: build a new dictionary and install a new feature, rather than mutating the + // existing Value in place (which would race with the outgoing encoding). + Pipeline pipeline = new Pipeline() + .Use(next => new InlineInvoker((request, cancellationToken) => + { + IRequestContextFeature? feature = request.Features.Get(); + IDictionary current = + feature?.Value ?? new Dictionary(); + var updated = new Dictionary(current) + { + ["CorrelationId"] = Guid.NewGuid().ToString() + }; + request.Features = request.Features.With( + new RequestContextFeature(updated)); + return next.InvokeAsync(request, cancellationToken); + })) + .UseRequestContext(); + #endregion + } } diff --git a/src/IceRpc/Features/IRequestContextFeature.cs b/src/IceRpc/Features/IRequestContextFeature.cs index a8dff748ba..6031971f05 100644 --- a/src/IceRpc/Features/IRequestContextFeature.cs +++ b/src/IceRpc/Features/IRequestContextFeature.cs @@ -4,6 +4,15 @@ namespace IceRpc.Features; /// Represents a feature that holds a of strings. This feature can be /// transmitted as a field with both ice and icerpc. +/// The outgoing request context is encoded lazily, when the request is sent. To avoid racing mutations +/// with encoding, an interceptor that needs to add or remove context entries should build a new dictionary and +/// install a new , rather than mutating in place. +/// +/// +/// The following code shows how to update the request context from an interceptor using copy-on-write. +/// +/// public interface IRequestContextFeature { /// Gets the value of this feature. diff --git a/src/IceRpc/Features/RequestContextFeature.cs b/src/IceRpc/Features/RequestContextFeature.cs index 20bb30bbf9..7593b22880 100644 --- a/src/IceRpc/Features/RequestContextFeature.cs +++ b/src/IceRpc/Features/RequestContextFeature.cs @@ -24,9 +24,11 @@ public string this[string key] /// Constructs an empty writeable request context feature. public RequestContextFeature() => Value = new Dictionary(); - /// Constructs a request context feature with the specified dictionary. - /// The dictionary that the new feature will hold. - public RequestContextFeature(IDictionary dictionary) => Value = dictionary; + /// Constructs a request context feature with a copy of the specified dictionary. + /// The source dictionary. Its entries are copied into the new feature so that later + /// mutations of do not affect this feature. + public RequestContextFeature(IDictionary dictionary) => + Value = new Dictionary(dictionary); /// Adds a new entry to . /// The key of the new entry. diff --git a/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs b/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs index b5dcc2a479..bcf426ead2 100644 --- a/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs +++ b/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs @@ -50,6 +50,19 @@ public async Task Context_feature_encoded_in_context_field() Assert.That(decoded, Is.EqualTo(context)); } + [Test] + public void Feature_is_isolated_from_mutations_to_source_dictionary() + { + var context = new Dictionary { ["Foo"] = "Bar" }; + var feature = new RequestContextFeature(context); + + context["Foo"] = "Mutated"; + context["Added"] = "After"; + context.Remove("Foo"); + + Assert.That(feature.Value, Is.EqualTo(new Dictionary { ["Foo"] = "Bar" })); + } + [Test] public async Task Empty_context_not_encoded_in_context_field() { From 3c051be04448a0bae8fdc7a4903665f8132b29e5 Mon Sep 17 00:00:00 2001 From: Jose Date: Mon, 27 Apr 2026 13:35:02 +0200 Subject: [PATCH 2/2] Document the no-copy contract instead of snapshotting the dictionary Per review, putting the snapshot responsibility on the caller is preferable to making the feature copy on every construction (the copy would also drop a custom key comparer if the caller passed one). Revert the constructor to wrap the dictionary by reference and tighten the doc comment to flag the contract: callers must not mutate the dictionary after handing it to the feature, since the request context is encoded lazily. The IRequestContextFeature remarks and docfx example continue to demonstrate the copy-on-write update pattern. Drop the regression test that asserted isolation (we are intentionally not isolated now), and discard the unused Pipeline local in the docfx example so it builds with TreatWarningsAsErrors. --- .../RequestContextInterceptorExamples.cs | 2 +- src/IceRpc/Features/RequestContextFeature.cs | 11 ++++++----- .../RequestContextInterceptorTests.cs | 13 ------------- 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs b/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs index 68ff18ac3a..655786f77b 100644 --- a/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs +++ b/docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs @@ -55,7 +55,7 @@ public static void UpdateRequestContextInInterceptor() // An interceptor that adds an entry to the request context. The appropriate pattern is // copy-on-write: build a new dictionary and install a new feature, rather than mutating the // existing Value in place (which would race with the outgoing encoding). - Pipeline pipeline = new Pipeline() + _ = new Pipeline() .Use(next => new InlineInvoker((request, cancellationToken) => { IRequestContextFeature? feature = request.Features.Get(); diff --git a/src/IceRpc/Features/RequestContextFeature.cs b/src/IceRpc/Features/RequestContextFeature.cs index 7593b22880..0c038904aa 100644 --- a/src/IceRpc/Features/RequestContextFeature.cs +++ b/src/IceRpc/Features/RequestContextFeature.cs @@ -24,11 +24,12 @@ public string this[string key] /// Constructs an empty writeable request context feature. public RequestContextFeature() => Value = new Dictionary(); - /// Constructs a request context feature with a copy of the specified dictionary. - /// The source dictionary. Its entries are copied into the new feature so that later - /// mutations of do not affect this feature. - public RequestContextFeature(IDictionary dictionary) => - Value = new Dictionary(dictionary); + /// Constructs a request context feature wrapping the specified dictionary. + /// The dictionary that the new feature will hold. The feature stores the reference; it + /// does not copy. The caller must not mutate after constructing the feature, since + /// the request context is encoded lazily when the request is sent. See + /// for the recommended copy-on-write update pattern. + public RequestContextFeature(IDictionary dictionary) => Value = dictionary; /// Adds a new entry to . /// The key of the new entry. diff --git a/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs b/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs index bcf426ead2..b5dcc2a479 100644 --- a/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs +++ b/tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs @@ -50,19 +50,6 @@ public async Task Context_feature_encoded_in_context_field() Assert.That(decoded, Is.EqualTo(context)); } - [Test] - public void Feature_is_isolated_from_mutations_to_source_dictionary() - { - var context = new Dictionary { ["Foo"] = "Bar" }; - var feature = new RequestContextFeature(context); - - context["Foo"] = "Mutated"; - context["Added"] = "After"; - context.Remove("Foo"); - - Assert.That(feature.Value, Is.EqualTo(new Dictionary { ["Foo"] = "Bar" })); - } - [Test] public async Task Empty_context_not_encoded_in_context_field() {