Document the no-copy contract on RequestContextFeature#4542
Open
pepone wants to merge 2 commits intoicerpc:mainfrom
Open
Document the no-copy contract on RequestContextFeature#4542pepone wants to merge 2 commits intoicerpc:mainfrom
pepone wants to merge 2 commits intoicerpc:mainfrom
Conversation
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 icerpc#4454
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency/correctness hazard where outbound request context encoding could observe later mutations of a caller-provided dictionary by snapshotting the dictionary when constructing RequestContextFeature.
Changes:
- Make
RequestContextFeature(IDictionary<string,string>)create a defensive copy of the source dictionary. - Document the recommended copy-on-write update pattern for request context in
IRequestContextFeature(with a docfx-linked example). - Add a regression test ensuring the feature is isolated from subsequent mutations of the source dictionary.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/IceRpc.RequestContext.Tests/RequestContextInterceptorTests.cs | Adds a regression test asserting the feature doesn’t reflect later source dictionary mutations. |
| src/IceRpc/Features/RequestContextFeature.cs | Updates ctor to snapshot/copy the provided dictionary. |
| src/IceRpc/Features/IRequestContextFeature.cs | Adds remarks and docfx example reference describing copy-on-write usage. |
| docfx/examples/IceRpc.RequestContext.Examples/RequestContextInterceptorExamples.cs | Adds the referenced docfx example region demonstrating copy-on-write updates in an interceptor. |
pepone
commented
Apr 23, 2026
| /// <summary>Constructs a request context feature with a copy of the specified dictionary.</summary> | ||
| /// <param name="dictionary">The source dictionary. Its entries are copied into the new feature so that later | ||
| /// mutations of <paramref name="dictionary"/> do not affect this feature.</param> | ||
| public RequestContextFeature(IDictionary<string, string> dictionary) => |
Member
Author
There was a problem hiding this comment.
I'm not 100% sure of adding this defensive copy, maybe the better fix is to clearly document this ..., if you are going to modify just pass a copy.
Member
There was a problem hiding this comment.
Yes, I put off reviewing this because I also wasn't sure about making this copy 100% of the time...
I think it'd be better to put this responsibility on the user, instead of making a copy.
Member
Author
There was a problem hiding this comment.
Reworked this PR it just update the docs, and includes an example for the correct mutation pattern.
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.
pepone
changed the title
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.
The outgoing request context is encoded lazily, when the request is sent. If the caller continues mutating the dictionary it handed to
RequestContextFeatureafter install, those mutations can race with the encoding and change what gets written on the wire (#4454).Original approach was to snapshot the dictionary in the constructor. Per review (the snapshot would also drop a custom key comparer if one was supplied, and forces an allocation on every construction), pivot to document the no-copy contract instead and put the snapshot responsibility on the caller:
RequestContextFeature(IDictionary)doc comment to state that the feature stores the reference (no copy), and the caller must not mutate the dictionary after handing it over.<remarks>and a copy-on-write<example>onIRequestContextFeatureshowing how an interceptor that needs to add or remove entries should build a fresh dictionary and install a new feature, rather than mutatingValuein place.Fixes #4454