Repository navigation
Prevent duplicate pooled buffer returns - #3600
Merged
Merged
Conversation
Remove duplicate stream ownership in ODataJsonValueSerializer. EndStreamValueScope and EndStreamValueScopeAsync are now solely responsible for disposing writer-owned streams, preventing the serializer and writer from returning the same pooled byte array twice. Make binary stream and text writer disposal idempotent across synchronous, asynchronous, repeated, and mixed disposal paths. Interlocked disposal gates and atomic ownership detachment guarantee that each rented byte or character array is returned at most once and that repeated scope completion cannot reuse an already-disposed writer. This prevents duplicate references from entering a shared ArrayPool, where simultaneous later rentals could otherwise alias the same array and cause cross-operation data corruption or disclosure. Use nested try/finally cleanup so writer scopes are completed when copying fails and source streams still honor ODataBinaryStreamValue.LeaveOpen. Preserve ConfigureAwait(false) for asynchronous library cleanup. Add internal injectable array pools for deterministic ownership tests while production constructors continue using the shared pools. Cover exact rent/return accounting, repeated and mixed disposal, serializer ownership, and Base64 boundary payload sizes 1, 2, 3, 2048, and 2049.
Contributor
Author
|
/AzurePipelines run |
There was a problem hiding this comment.
Pull request overview
This PR hardens JSON streaming/text-writing paths against duplicate buffer returns by making writer-owned stream/text-writer disposal idempotent and by moving stream lifetime responsibility fully into EndStreamValueScope{Async}. It also adds targeted unit tests to validate that pooled buffers and writer-owned streams are disposed/returned exactly once across repeated and mixed sync/async disposal paths.
Changes:
- Make
ODataUtf8JsonWriteStreamandODataUtf8JsonTextWriterdisposal idempotent (sync/async) using an interlocked disposal gate and atomic buffer detachment, and allow injectingArrayPool<T>for deterministic tests. - Ensure
ODataUtf8JsonWriterscope-ending methods detach writer-owned streams/text-writers before disposing them, preventing repeated scope completion from double-disposing. - Update
ODataJsonValueSerializersync/async stream writing to always callEndStreamValueScope{Async}viatry/finally, while still honoringODataBinaryStreamValue.LeaveOpen, and add tests verifying “dispose once” semantics.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.OData.Core.Tests/Json/ODataUtf8JsonWriterStreamTests.cs | Adds buffer rent/return accounting tests for repeated and mixed disposal of ODataUtf8JsonWriteStream, plus a tracking ArrayPool<T>. |
| test/UnitTests/Microsoft.OData.Core.Tests/Json/ODataUtf8JsonTextWriterTests.cs | Adds mixed sync/async disposal test for ODataUtf8JsonTextWriter buffer return idempotency. |
| test/UnitTests/Microsoft.OData.Core.Tests/Json/ODataJsonValueSerializerTests.cs | Adds a regression test verifying writer-owned stream is disposed exactly once during WriteStreamValue. |
| test/UnitTests/Microsoft.OData.Core.Tests/Json/ODataJsonValueSerializerAsyncTests.cs | Adds an async regression test verifying writer-owned stream is disposed exactly once during WriteStreamValueAsync. |
| test/UnitTests/Microsoft.OData.Core.Tests/Json/MockJsonWriter.cs | Extends the mock to support stream scope start/end (sync/async) so disposal behavior can be asserted. |
| src/Microsoft.OData.Core/Json/ODataUtf8JsonWriter.TextWriter.cs | Makes writer-owned text-writer scope completion idempotent; adds injectable char pool and interlocked disposal gate to prevent double returns. |
| src/Microsoft.OData.Core/Json/ODataUtf8JsonWriter.Stream.cs | Makes writer-owned stream scope completion idempotent; adds injectable byte pool and interlocked disposal gate to prevent double returns. |
| src/Microsoft.OData.Core/Json/ODataJsonValueSerializer.cs | Removes duplicate ownership/disposal of writer-owned streams and ensures scope end is always invoked (sync/async) via try/finally. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Remove duplicate stream ownership in ODataJsonValueSerializer. EndStreamValueScope and EndStreamValueScopeAsync are now solely responsible for disposing writer-owned streams, preventing the serializer and writer from returning the same pooled byte array twice.
Make binary stream and text writer disposal idempotent across synchronous, asynchronous, repeated, and mixed disposal paths. Interlocked disposal gates and atomic ownership detachment guarantee that each rented byte or character array is returned at most once and that repeated scope completion cannot reuse an already-disposed writer.
This prevents duplicate references from entering a shared ArrayPool, where simultaneous later rentals could otherwise alias the same array and cause cross-operation data corruption or disclosure.
Use nested try/finally cleanup so writer scopes are completed when copying fails and source streams still honor ODataBinaryStreamValue.LeaveOpen. Preserve ConfigureAwait(false) for asynchronous library cleanup.
Add internal injectable array pools for deterministic ownership tests while production constructors continue using the shared pools. Cover exact rent/return accounting, repeated and mixed disposal, serializer ownership, and Base64 boundary payload sizes 1, 2, 3, 2048, and 2049.
Issues
This pull request fixes #xxx.
Description
Briefly describe the changes of this pull request.
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.
Repository notes
Team members can start a CI build by adding a comment with the text
/AzurePipelines runto a PR. A bot may respond indicating that there is no pipeline associated with the pull request. This can be ignored if the build is triggered.Team members should not trigger a build this way for pull requests coming from forked repositories. They should instead trigger the build manually by setting the "branch" to
refs/pull/{prId}/mergewhere{prId}is the ID of the PR.