Repository navigation
feat: Incremental CBOR reader adoption - #4555
Conversation
| { | ||
| return readOperation(this, _internalCborReader); | ||
| } | ||
| catch (CborContentException) when (!_streamExhausted) |
There was a problem hiding this comment.
If the payload is actually malformed (not just truncated), this keeps refilling and retrying until the stream is exhausted, so the whole response ends up in the buffer before we throw.
Worth checking whether the retry is needed for anything other than SkipValue and indefinite-length strings, since PeekState already guarantees single-token reads won't fail from truncation (SkipValue has TrySkipValue in the new API).
Example test:
[Fact]
public void MalformedByte_DoesNotBufferRestOfStream()
{
// [ 0x1C (reserved additional info), then 2 MB of padding ]
var bytes = new byte[2 * 1024 * 1024];
bytes[0] = 0x82; bytes[1] = 0x1C;
var stream = new MemoryStream(bytes);
using var reader = new CborStreamReader(stream);
reader.ReadStartArray();
Assert.Throws<CborContentException>(() => reader.ReadInt32());
Assert.True(stream.Position < 2 * AWSConfigs.CborReaderInitialBufferSize);
}There was a problem hiding this comment.
Good catch. Rewrote ExecuteRead to drive refills proactively off PeekState() == NeedsMoreData and removed the catch (CborContentException) when (!_streamExhausted) retry. A content error now propagates immediately instead of refilling to the end of the stream, so a malformed token fails fast.
For the two cases you flagged: single-token reads are covered by the proactive NeedsMoreData loop, and SkipValue now uses TrySkipValue in a refill loop (returns false without consuming when it needs more data), so neither path depends on exceptions for control flow now.
I added your MalformedByte_DoesNotBufferRestOfStream test (adapted to the test fixture's buffer size) & it passes: on the malformed byte the stream position and buffer stay small rather than draining the 2 MB.
|
Also as mentioned dry run for .NET is required on this one especially if changing the benchmarks to .NET 11. Also verify the changed tests are being run by the build system under non .NET framework targets. |
c43cca5 to
3242f07
Compare
| Backward compatibility review required - approval needed from one of the reviewers: normj, boblodgett |
The incremental reader is no longer gated on net11, so every target needs the 11.0 package. The NetFramework project and the nuspec still referenced 9.0.5, which broke the net472 build and would have shipped a 9.0.5 dependency.
System.Formats.Cbor 11.0 depends on System.ValueTuple 4.6.2, which treats net471+ as in-box, so net472 no longer publishes System.ValueTuple.dll. It also has no net8.0 asset, so net8.0 uses the netstandard2.0 build and now publishes Microsoft.Bcl.HashCode.dll.
The incremental reader is no longer gated on net11, so a net10.0 runner already exercises it through System.Formats.Cbor 11.0. The Catapult perf test hosts only have the .NET 10 runtime, so a net11.0 runner fails to start.
|
Dry-run succeeded in v4.1 dev stack. |
PeekState only waits for the next item's header, so indefinite-length strings and decimal fractions could still run past the buffered chunk and throw. For values starting with 0x5F, 0x7F or 0xC4, refill and retry until the read succeeds or the stream is exhausted. Adds boundary-sweep and truncation tests.
Description
Adopt the .NET 11 incremental
CborReaderAPIs for CBOR deserialization. Read path only; serialization is untouched.Incremental decode path on all targets.
CborStreamReadernow usesCborReader.SlideData(...)andCborReaderState.NeedsMoreDatafromSystem.Formats.Cbor11.0, which keep parsing state across chunk boundaries, replacing the hand-rolled buffering. The package shipsnet462,netstandard2.0,net10.0andnet11.0assets, so every target uses the same reader (net8.0 gets thenetstandard2.0asset).Refills. Reads refill while the reader reports
NeedsMoreData;SkipValuerefills in aTrySkipValueloop. Indefinite-length strings and decimal fractions span several CBOR items, so they can still cross a chunk edge after that check; for those (initial byte0x5F,0x7For0xC4) a failed read is retried after a refill until it succeeds or the stream is exhausted. Any other content error propagates.Pooled buffer.
Fill()moves unconsumed bytes to the front of a pooledArrayPool<byte>buffer and appends the next chunk, so the reader never sees a split item. The buffer only grows when unconsumed bytes already fill it, so a malformed length header can't force a large allocation.Lax conformance. Required for incremental reads (stricter modes reject a non-final block), with
AllowMultipleRootLevelValues = true. The stream is read lazily, so an event-stream response is handed on undecoded.Errors. The reader's
InvalidOperationExceptionfor structural misuse on a malformed payload is wrapped asCborContentException, matching the previous reader.Package reference.
System.Formats.Cbormoves from9.0.5to11.0.0-rc.2.26504.105on all targets (both csproj files and the nuspec). Move to11.0.0once .NET 11 GA is on nuget.org.Performance
Numbers are the AWSJSON mean / CBOR mean ratio within a single run (>1.00 = CBOR faster), which cancels out runtime and machine differences. Same harness and instance types for all four runs:
feature/4.1: old reader,System.Formats.Cbor9.0.5).System.Formats.Cbor11.0 RC2).System.Formats.Cbor11.0 RC2, incremental path forced off (old reader).The net11 pair differs only in reader code. The net10 pair also differs in package version, so it shows what a .NET 10 consumer gets on upgrade. The net10 pair was measured at
c487235e5e2d; the net11 pair is from earlier runs.m7i.xlarge (x86) — deserialization
m7g.xlarge (arm) — deserialization
Headline: deserialization improves on both runtimes, across all
GetItemOutput*sizes and payload types:The change wins by a similar margin within each runtime, so the gain comes from the reader, not .NET 11.
Serialization is unchanged (write path untouched):
PutItemRequest_*ratios stay within run-to-run noise in both pairs (±6% on net10).Motivation and Context
CborStreamReaderhand-rolled its buffering becauseSystem.Formats.Cborhad no incremental API. The 11.0SlideData/NeedsMoreDataAPIs remove that workaround on every target and make CBOR deserialization measurably faster relative to AWSJSON.Testing
CborStreamReaderTests: lazy element-count validation (a declared array count is returned, the error surfaces when the body runs out, and a header can't force a ~100 MB allocation); boundary sweeps that place an indefinite-length text string, byte string and decimal at every offset 0–399 with a 100-byte buffer; and truncation inside each of those values.net11.0and the old reader forced off for the base, holding runtime and package constant. The net10 pair ranfeature/4.1(a0103d77f9a2) and this branch (c487235e5e2d) back to back on the .NET 10.0.14 runtime. The runner (SdkPerformanceTargets) stays onnet10.0, since Catapult perf hosts only have the .NET 10 runtime.Dry-runs
DotNet Dry-run ID: DRY_RUN-9ff02a83-9140-4402-a0e6-484173d41ba6 (my dev account)
PowerShell Dry-run ID: DRY_RUN-cdb6476f-8476-4d31-ae43-4efa82d524ef (my dev account)
Breaking Changes Assessment
CborReaderAPIs on all targets;System.Formats.Cbormoves from9.0.5to11.0.0-rc.2.26504.105. Returned values and exception types are unchanged.System.Formats.Cbordependency.ArrayPool<byte>buffer, bounded growth, and static (non-capturing) read delegates.Screenshots (if appropriate)
N/A
Types of changes
Checklist
License