CBOR response unmarshallers and related protocol test #3905

muhammad-othman · 2025-07-07T18:16:08Z

Description

Implement Cbor response unmarshallers.
Implement Cbor structure unmarshallers.
Add shared base classes like CborUnmarshallerContext and CborResponseUnmarshaller to AWSSDK.Extensions.CborProtocol package.
Generate CBOR response protocol tests.
Didn't need to CloudWatch or SecretsManager unmarshallers this time since we have the protocol tests unmarshallers for reviewing the generator output.

Motivation and Context

DOTNET-8161
DOTNET-8162
DOTNET-8165

Testing

Added smithy-rpc-v2-cbor to CloudWatch and SecretsManager models protocols, generated the marshallers/unmarshallers, and executed some requests using CBOR marshallers and unmarshallers.
CBOR response protocol tests.
DRY_RUN-36078c9c-efcb-46d7-95e3-a6052e91aa33.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

License

I confirm that this pull request can be released under the Apache 2 license

peterrsongg · 2025-07-08T18:54:50Z

...t-codegen/src/main/java/software/amazon/smithy/dotnet/codegen/HttpProtocolTestGenerator.java

@@ -98,7 +98,7 @@ private void addServiceProtocolSpecificImports() {
            writer.addImport(serviceName, "System.Xml");
            writer.addImport(serviceName, "System.Xml.Linq");
        } else if (this.serviceName.toLowerCase().contains("rpcv2")) {
-            writer.addImport(serviceName, "AWSSDK.Extensions.CborProtocol.Internal");
+            writer.addImport(serviceName, "AWSSDK.Extensions.CborProtocol.Internal.Transform");


This is just a general comment, but I don't see the "QueryCompatbile" protocol tests added here. The CBOR protocol-selection SEP says that a service using the CBOR protocol can make use of the awsquerycompatible trait. I'm fine with adding this in a separate PR but I was wondering if that was on your radar

query-compatbile tests

Looks like these were added two weeks ago, after I added the protocol tests, will include them in the next PR.

peterrsongg · 2025-07-08T19:09:11Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborResponseUnmarshaller.cs

+        {
+            return new CborUnmarshallerContext(responseStream, maintainResponseBody, response, isException, requestContext);
+        }
+    }


Do we need to override

ShouldReadEntireResponse(IWebResponseData response, bool readEntireResponse)

It is defined in the base class so I'm just wondering if you left it out intentionally

Yeah, didn't need to override it here since we are reading the response anyway for the CborReader.

peterrsongg · 2025-07-08T19:20:33Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborUnmarshallerContext.cs

+
+                // Create a new byte array to hold only the read data.
+                var actualBytes = new byte[totalRead];
+                Buffer.BlockCopy(tempBuffer, 0, actualBytes, 0, totalRead);


doesn't tempBuffer already contain all the data you need? Why do you need to create another byte array and copy the contents of tempBuffer to actualBytes?

When we use ArrayPool<byte>.Shared.Rent(someSize) it may give us a buffer of larger size, if we passed this to CborReader it wont know that it should reading data after the size that we specified, so we have to copy only the data that we need to CborReader.
I wasn't sure about this whole approach at the beginning since we allocate some additional memory just to pass it to CborReader, but I did some benchmarking and even with that it still allocates less memory than JSON, and much less than XML

Ah okay, i remember running into this as well when using array pools. makes sense.

Instead of copying the whole value can't we just wrap the byte buffer in a properly sized ReadOnlyMemory struct? That would avoid the copying?

var actualBytes = new ReadOnlyMemory<byte>(tempBuffer, 0, totalRead);

When we do that we would not return the tempBuffer to the array pool since passed the buffer to another object.

This could be a good improvement, but I guess we should keep a reference to the tempBuffer then return it when we dispose the Unmarshaller, otherwise we may end up with unnecessary allocations.
Will do some testing and make this change.

@normj Updated the final copy to use ReadOnlyMemory and I can see some improvement in the memory allocations over the previous approach.

normj · 2025-07-14T16:29:57Z

...sions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborErrorResponseUnmarshaller.cs

+using System.Formats.Cbor;
+using System.IO;
+
+namespace AWSSDK.Extensions.CborProtocol.Internal.Transform


Part of weird legacy is we never use or should never use AWSSDK in the namespace name. For example AWSSDK.Extensions.NETCore.Setup uses Amazon.Extension.NETCore.Setup as the namespace. I know it is weird but I want to be consistent and not have 1% of code use a different namespace.

Updating the namespace.

Updated the namespaces to Amazon.Extensions.CborProtocol.

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborSimpleTypeUnmarshaller.cs

normj · 2025-07-14T17:07:48Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborSimpleTypeUnmarshaller.cs

+            )
+            {
+                reader.ReadNull();
+                value = null;


Should value be set to default(T) so types like int would be 0?

int cannot be null and reader.PeekState() == CborReaderState.Null wont be true, this is related to the above comment and being extra protective.
But if we kept the the typeof conditions then returning default(T) will give the same result as we made sure the type is nullable, but I feel it is more clear that way (we read null and we return null) but since it is the same output I'm okay with changing it.

Switched to default(T).

normj · 2025-07-14T17:29:52Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborUnmarshallerContext.cs

+
+                // Create a new byte array to hold only the read data.
+                var actualBytes = new byte[totalRead];
+                Buffer.BlockCopy(tempBuffer, 0, actualBytes, 0, totalRead);


Instead of copying the whole value can't we just wrap the byte buffer in a properly sized ReadOnlyMemory struct? That would avoid the copying?

var actualBytes = new ReadOnlyMemory<byte>(tempBuffer, 0, totalRead);

When we do that we would not return the tempBuffer to the array pool since passed the buffer to another object.

normj · 2025-07-14T21:54:25Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborUnmarshallerContext.cs

+        {
+            if (!this.disposed)
+            {
+                if (disposing && rentedBuffer != null)


The member variable rentedBuffer is never set and is always null because CreateCborReaderFromStream which is static is working with a local rentedBuffer variable. Some logic still needs to be fixed up for the ArrayPool<byte>.Shared.Return to work here.

lol, how did I get some allocations improvements 🤔

Aha, I guess we get the benefits of ReadOnlyMemory even if we didn't return the buffer

Updated it to set the private member, gonna compare it to the previous version to see if returning the memory made any difference.

normj · 2025-07-15T17:10:12Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborUnmarshallerContext.cs

+                    }
+                    catch
+                    {
+                        ArrayPool<byte>.Shared.Return(newBuffer);


Since we are exiting out due to an exception shouldn't we also Return the rentedBuffer as well?

normj · 2025-07-15T17:11:33Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/Transform/CborUnmarshallerContext.cs

+            if (streamSize.HasValue)
+            {
+                // If we know the size, we can read directly into a buffer of exact size
+                var buffer = new byte[streamSize.Value];


Can we do the same trick as below and rent a buffer here and return as part of the dispose?

Trying this made me think that we will have an issue if the stream size is larger than int.MaxValue since arrays size should be an int, in JSON and XML where we can read chunks directly from the stream so it doesn't matter if we have a stream larger than 2GB, but here the CborReader needs the whole buffer since it suppose to improve its performance being in-memory parser.
I'm not sure if a Cbor payload can be larger than that, will double check.

Implement Cbor response and structure unmarshallers generators

2ece81a

philasmar requested review from peterrsongg and normj July 8, 2025 17:27

peterrsongg reviewed Jul 8, 2025

View reviewed changes

peterrsongg approved these changes Jul 9, 2025

View reviewed changes

normj requested changes Jul 14, 2025

View reviewed changes

muhammad-othman force-pushed the muhamoth/DOTNET-8161-cbor-response-unmarshallers branch from d5ea8b4 to 241892b Compare July 14, 2025 21:03

muhammad-othman requested a review from normj July 14, 2025 21:05

normj reviewed Jul 14, 2025

View reviewed changes

muhammad-othman force-pushed the muhamoth/DOTNET-8161-cbor-response-unmarshallers branch from 241892b to 2ece81a Compare July 14, 2025 21:59

muhammad-othman requested a review from normj July 14, 2025 22:02

muhammad-othman mentioned this pull request Jul 14, 2025

Updated the build to package AWSSDK.Extensions.CborProtocol and notice file updates. #3914

Merged

10 tasks

normj requested changes Jul 15, 2025

View reviewed changes

Generate CBOR response protocol tests

0a8c9d5

muhammad-othman force-pushed the muhamoth/DOTNET-8161-cbor-response-unmarshallers branch from 465b51f to 0a8c9d5 Compare July 15, 2025 22:18

normj approved these changes Jul 15, 2025

View reviewed changes

muhammad-othman merged commit ebca005 into cbor-protocol Jul 15, 2025
1 check passed

dscpinheiro deleted the muhamoth/DOTNET-8161-cbor-response-unmarshallers branch July 16, 2025 23:38

muhammad-othman mentioned this pull request Aug 1, 2025

Add CBOR Protocol Support for Request Marshalling and Response Unmarshalling #3942

Draft

10 tasks

CBOR response unmarshallers and related protocol test #3905

CBOR response unmarshallers and related protocol test #3905

Uh oh!

Conversation

muhammad-othman commented Jul 7, 2025

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!