Conversation
Implements the API approved in dotnet#129718: adding incremental reading mode where the caller owns I/O and supplies data in chunks via SlideData, with PeekState() reporting NeedsMoreData on incomplete tokens and TrySkipValue/TrySkipToParent returning false instead of throwing. Non-final mode requires CborConformanceMode.Lax, since the other modes retain map key encodings as buffer offsets that cannot survive a slide. Final-block readers are unchanged. Fix dotnet#129718
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-formats-cbor, @bartonjs, @vcsjones |
|
@dotnet-policy-service agree company="Amazon" |
There was a problem hiding this comment.
🔵 Needs a closer look
It adds new public API surface and changes core reader behavior, which warrants final maintainer review despite strong accompanying test coverage.
Pull request overview
This PR extends System.Formats.Cbor.CborReader with an incremental (chunked) reading mode where callers can supply data in multiple buffers, while preserving nesting state between refills.
Changes:
- Adds new public APIs to support incremental reading (
isFinalBlockconstructor/Reset,SlideData,TrySkipValue/TrySkipToParent) and introducesCborReaderState.NeedsMoreData. - Updates
PeekState()so non-final readers returnNeedsMoreDatafor truncated next-token scenarios (instead of throwing), while multi-token semantic tag readers continue to throw on truncation and roll back state. - Adds/updates tests to validate
SlideDatasemantics,NeedsMoreDatagating behavior, and resumable skipping across all split points.
File summaries
| File | Description |
|---|---|
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.cs | Adds isFinalBlock plumbing, SlideData, Reset overload, and checkpoint/state support needed for incremental mode. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.PeekState.cs | Implements NeedsMoreData behavior for non-final readers and token-availability gating. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.SkipValue.cs | Introduces TrySkipValue/TrySkipToParent and reworks skipping to return false for needs-more-data in non-final mode. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.Integer.cs | Adds DecodeCollectionLength to relax definite-length collection header handling for non-final buffers. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.Array.cs | Uses DecodeCollectionLength so array headers can be read even when contents arrive later. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.Map.cs | Uses DecodeCollectionLength and adjusts definite-length map size validation for non-final mode. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.Tag.cs | Ensures semantic tag readers treat NeedsMoreData as truncation (throw) to preserve atomic multi-token semantics. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReader.String.cs | Documents that returned memory views can be invalidated if the caller reuses buffers via SlideData. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Reader/CborReaderState.cs | Adds CborReaderState.NeedsMoreData with API docs. |
| src/libraries/System.Formats.Cbor/src/Resources/Strings.resx | Adds localized strings for new incremental-mode validation and exceptions. |
| src/libraries/System.Formats.Cbor/ref/System.Formats.Cbor.cs | Updates public surface area to include the new APIs and enum member. |
| src/libraries/System.Formats.Cbor/tests/System.Formats.Cbor.Tests.csproj | Includes the new test file in the test build. |
| src/libraries/System.Formats.Cbor/tests/Reader/CborReaderTests.SlideData.cs | Adds extensive incremental-mode tests (sliding, gating, truncation behavior, resumability). |
| src/libraries/System.Formats.Cbor/tests/Reader/CborReaderTests.SkipValue.cs | Adds tests for TrySkipValue/TrySkipToParent behavior in non-final mode. |
| src/libraries/System.Formats.Cbor/tests/Reader/CborReaderTests.Helpers.cs | Expands test vectors and splits invalid inputs into truncated-vs-malformed sets for incremental semantics. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0
- Review effort level: Lite
bartonjs
left a comment
There was a problem hiding this comment.
Generally LGTM, but a few notes.
28502db to
b534825
Compare
muhammad-othman
left a comment
There was a problem hiding this comment.
Thank you for your review, I've applied your suggestions to the PR, please let me know if there are anything that I missed.
|
@artl93, was the servicing label meant for this PR? It's not a backport. Looks like a mistake. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Correct the NeedsMoreData documentation and malformed XML documentation before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (1)
Head branch was pushed to by a user without write access
8ccc8fa to
10210bb
Compare
|
@muhammad-othman No more commits, please (and please, never force-push to an already open PR). Every time you commit again you reset the CI clock, which is measured in hours. |
Sorry I was trying to get the commit history in good shape 😓 , no more commits from my side. |
|
@muhammad-othman We squash-merge, so all of the individual commits more or less go away at merge time. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Resolve the public overload ambiguity and address the documented skip and allocation issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
|
/ba-g Tests have already succeeded on multiple platforms, no platform-specific dependencies |
|
/backport to release/11.0 |
|
Started backporting to |
Backport of #133602 to release/11.0 /cc @bartonjs @muhammad-othman ## Customer Impact - [X] Customer reported - [ ] Found internally New API for .NET 11 to support processing a CBOR payload without forcing the whole document into contiguous memory. ## Regression - [ ] Yes - [X] No ## Testing Existing tests show that the existing "non-streaming" model works exactly as before, and the new tests confirm the behavior of the new "streaming" model. ## Risk Low. The test coverage says existing callers are unaffected, and the new behaviors are only visible to callers who use the new API. Co-authored-by: Muhammad Othman <muhammmadothman@gmail.com>


Implements the API approved in #129718, adding incremental reading mode where the caller owns I/O and supplies data in chunks:
PeekState()returnsNeedsMoreDatawhen the next token is incomplete in the buffer; any other state guarantees the single-token read won't fail on end of data. Multi-token reads (indefinite-length strings,ReadEncodedValue, semantic tag readers) still throw on truncation and restore the reader state, so the caller can slide in more data and retry.SlideDatareplaces the buffer and preserves nesting state; the caller must keep the unread bytes (BytesRemaining) at the start of the new buffer. Only the length is validated.Reset(data)) are behaviorally unchanged.Notes for reviewers:
CborContentExceptionrather thanInvalidOperationException, since the reader can't know the sequence ended until the final block arrives.Finished, though it isn't well-formed. This predates this change; incremental readers deliberately match it for consistency. I think we should track it separately and will create an issue for it.