Added support for binding the raw request body #39388

davidfowl · 2022-01-09T05:43:03Z

Added support for PipeReader, Stream, ~~ReadOnlySequence, ReadOnlyMemory and byte[]~~
Added test

~~This will potentially break custom JSON converters for byte[] (maybe base64 JSON encoded content?)~~

~~TBD: Fix the error messages that talk about JSON binding when these types are being used.~~

- Added support for Stream, ReadOnlySequence<byte>, ReadOnlyMemory<byte> and byte[] - Added test

JamesNK · 2022-01-09T08:48:27Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+                                // REVIEW: Does this need to be a copy? We can tell users to consume the buffer
+                                // immediately in the action
+                                return (buffer, true);


When is PipeReader.AdvanceTo called to say the body is consumed? Before or after the request delegate is executed?

After thinking about this, it's actually broken for non Kestrel servers since nobody completes that PipeReader currently. I'll need to add more logic there. Good catch.

Hmm actually no, it seems like we complete the pipe reader in both cases

aspnetcore/src/Http/Http/src/Features/RequestBodyPipeFeature.cs

Lines 35 to 48 in 694507a

if (_internalPipeReader == null ||

!ReferenceEquals(_streamInstanceWhenWrapped, _context.Request.Body))

{

_streamInstanceWhenWrapped = _context.Request.Body;

_internalPipeReader = PipeReader.Create(_context.Request.Body);

_context.Response.OnCompleted((self) =>

{

((PipeReader)self).Complete();

return Task.CompletedTask;

}, _internalPipeReader);

}

return _internalPipeReader;

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

- Advance the PipeReader preemptively in the cases where we copy the buffer. - Add support for PipeReader as an argument.

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

- Split the code path for raw body binding with ReadOnlySequence<byte> to avoid boxing.

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

halter73 · 2022-01-10T21:53:43Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+
+                    if (result.IsCompleted)
+                    {
+                        return buffer;


I think calling the route handler with the reader in an incomplete/unadvanced state is problematic. Any code accessing the BodyReader, either through the HttpContext or as a separate PipeReader parameter, will be unable to partially examine the body. Any attempt will throw an InvalidOperationException complaining that "The examined position cannot be less than the previously examined position."

I think we're better off just fully consuming the body if you take a ReadOnlySequence<byte> parameter.

I don’t want to copy but I appreciate the potential problems. I’d rather leave that with the caveats or cut the whole feature (which would make me sad but I’d get over it).

Given this endpoint:

app.MapPost("/read-twice", async (HttpContext httpContext, ReadOnlySequence<byte> body) => { await httpContext.Request.Body.ReadAsync(new byte[1], 0, 0); });

you get the following error:

System.InvalidOperationException: Reading is already in progress. at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.ReadAsyncInternal(CancellationToken cancellationToken) in C:\dev\dotnet\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Http\Http1ContentLengthMessageBody.cs:line 34 at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1.StateMachineBox`1.System.Threading.Tasks.Sources.IValueTaskSource<TResult>.GetResult(Int16 token) at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.ReadAsyncInternal(Memory`1 destination, CancellationToken cancellationToken) in C:\dev\dotnet\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Http\HttpRequestStream.cs:line 114 at Program.<>c.<<<Main>$>b__0_7>d.MoveNext() in C:\dev\dotnet\aspnetcore\src\Http\samples\MinimalSample\Program.cs:line 42 --- End of stack trace from previous location --- at Microsoft.AspNetCore.Http.RequestDelegateFactory.<>c__DisplayClass54_1.<<HandleRequestBodyAndCompileRequestDelegateForRawBody>b__2>d.MoveNext() in C:\dev\dotnet\aspnetcore\src\Http\Http.Extensions\src\RequestDelegateFactory.cs:line 647

I don't think this is acceptable. We should either:

Consume the entire body so this read returns 0 (edit: This would require a copy anyway to avoid referencing memory that's already returned to the pool, so at this point we might as well do option 2)

Buffer the entire body so you can actually read it again

Poison the body so that it throws a better error than System.InvalidOperationException: Reading is already in progress.

Cut the whole feature

OK I pushed another commit with option 3a. I set the body to Stream.Null. This fixes trying to consume the body after it's already been consumed by our binding logic.

pranavkm · 2022-01-10T21:54:37Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+
+            // Looping over arrays is faster
+            var binders = factoryContext.ParameterBinders.ToArray();
+            var count = binders.Length;


You could capture this inside the lamda, no?

I originally didn’t do it would only capture these locals only (in case we nulled out the factory context later).

pranavkm · 2022-01-10T21:55:38Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+
+    private static Func<object?, HttpContext, Task> HandleRequestBodyAndCompileRequestDelegateForRawBody(Expression responseWritingMethodCall, FactoryContext factoryContext)
+    {
+        Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter.");


Did you mean

Suggested change

Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter.");

Debug.Assert(factoryContext.RequestBodyParameter is null, "factoryContext.RequestBodyParameter is null for a body parameter.");

Don't think so. We're asserting that it isn't null

pranavkm · 2022-01-10T21:57:09Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+        if (factoryContext.ParameterBinders.Count > 0)
+        {
+            // We need to generate the code for reading from the body before calling into the delegate
+            var continuation = Expression.Lambda<Func<object?, HttpContext, ReadOnlySequence<byte>, object?[], Task>>(


Noob question - why is this task returning (vs ValueTask returning)?

This could be either but using Task vs ValueTask but there’s no benefit to using ValueTask here. There’s no pooling to be had and synchronous completion won’t allocate regardless

pranavkm · 2022-01-10T21:58:28Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+        {
+            var feature = httpContext.Features.Get<IHttpRequestBodyDetectionFeature>();
+
+            if (feature?.CanHaveBody == true)


What's the behavior with JSON if there isn't a body to read from? We should try and emulate that behavior.

If allow empty body is true it’ll bind otherwise 400

pranavkm · 2022-01-10T22:02:23Z

src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs

+                httpContext.Items.Add("body", body.ToArray());
+            }
+
+            void TestStream(HttpContext httpContext, Stream stream)


pranavkm · 2022-01-10T22:05:02Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

        public bool AllowEmptyRequestBody { get; set; }
+        public bool IsRawRequestBody => RequestBodyParameter?.ParameterType == typeof(ReadOnlySequence<byte>);


It'd be interesting to see if people actually start using it. It's a somewhat obtuse type to actually use (particularly when you can bind the PipeReader instead).

It’s something we’d have to advertise. I had support for ReadOnlyMemory<byte> and byte[] but removed them.

- This prevents code that would otherwise read the body from doing so after consuming it.

Tratcher · 2022-01-11T20:02:45Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+                        // We're not buffering the body so we want to block consuming code from reading again
+                        // and getting weird errors. Treat further reads as a fully consumed body.
+                        httpContext.Request.Body = Stream.Null;


This is odd, what if someone else is buffering the body?

I guess you'd have to check if the body is seekeable, if so seek it. RadFormAsync / MVC doesn't change / reset the Body once it has been read, couldn't we do the same here?

I think seeking given a seekable stream is fine.

If we're replacing the body anyway, we should advance the reader before returning even if Kestrel will do it anyway.

We would have to do this after invoking the route handler, so memory doesn't get returned too soon. I'm still worried about people keeping a reference to the ReadOnlySequence<byte> after the route handler completes if we don't copy.

If we're replacing the body anyway, we should advance the reader before returning even if Kestrel will do it anyway.

Just for cleanliness? It's not just Kestrel doing it, it would happen for any properly implemented pipereader (it will complete the pipereader).

I'm still worried about people keeping a reference to the ReadOnlySequence after the route handler completes if we don't copy.

Yea but this feature is about not copying the buffer. That's why it's a ROS<byte>. That's the one thing I'm unwilling to compromise here on as I think it makes the original scenario (posted in the issue) less efficient.

This is odd, what if someone else is buffering the body?

I don't see how that affects this? I can revert and set the the original stream after executing the handler. The body is being buffered for the logic that is consuming the buffer.

We have the reference to the pipe that we read and we can complete it. It's just redundant is all. Only a badly written application would end up never completing the pipe anyways. It would also need to be explicit written to do so.

It's more about generic code that might try to read any unconsumed bytes from the body after the route handler runs but before middleware completes.

Also @Tratcher I don't think we should check is seekable here. Specifically because we need to poison both the stream and the PipeReader to be unusable 😄. Also our buffering logic doesn't work if you don't read using the Stream AFAIK.

At least revert it when you're done then.

- Restore the original stream after the handler executes.

…m the call.

…ereader

davidfowl · 2022-01-20T04:13:25Z

This PR is ready now. I removed reading the request body as a raw buffer and am only supporting Stream and PipeReader.

BrennanConroy

Looks a lot simpler than before 😃

Do we have an issue to track doc updates for .NET 7? We need to add IFormFileCollection, PipeReader, and Stream to https://docs.microsoft.com/aspnet/core/fundamentals/minimal-apis?view=aspnetcore-6.0#special-types now

davidfowl · 2022-01-20T18:05:12Z

Do we have an issue to track doc updates for .NET 7? We need to add IFormFileCollection, PipeReader, and Stream to https://docs.microsoft.com/aspnet/core/fundamentals/minimal-apis?view=aspnetcore-6.0#special-types now

We don't have one yet. Good idea.

- Added support for Stream, and PipeReader - Added tests

Added support for binding the raw request body

daf6a86

- Added support for Stream, ReadOnlySequence<byte>, ReadOnlyMemory<byte> and byte[] - Added test

davidfowl requested review from BrennanConroy, halter73 and Tratcher as code owners January 9, 2022 05:43

ghost added the area-runtime label Jan 9, 2022

JamesNK reviewed Jan 9, 2022

View reviewed changes

src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved

davidfowl added 2 commits January 9, 2022 08:51

Fix some issues add more features

154abd1

- Advance the PipeReader preemptively in the cases where we copy the buffer. - Add support for PipeReader as an argument.

Random clean up

d08f132

davidfowl commented Jan 9, 2022

View reviewed changes

src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved

davidfowl added 5 commits January 9, 2022 13:12

Remove support for byte[] and ReadOnlyMemory<byte>

3e2fff8

Fix method names and remove extra fluff

547ce9b

Removed extra space

c7d1168

Removed JSON from the error message

8a17a3b

Formatting

34244b1

davidfowl commented Jan 9, 2022

View reviewed changes

src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved

davidfowl added 2 commits January 9, 2022 16:19

Remove the boxing for raw body binding

7c9b45e

- Split the code path for raw body binding with ReadOnlySequence<byte> to avoid boxing.

Added more tests

d2085fd

davidfowl commented Jan 10, 2022

View reviewed changes

src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved

davidfowl added 2 commits January 9, 2022 16:36

Small nit

f45db2b

Added more tests

a0711dc

martincostello reviewed Jan 10, 2022

View reviewed changes

src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved

Remove tuple and use ValueTask instead of Task

caf7d66

davidfowl requested a review from a team January 10, 2022 20:51

halter73 reviewed Jan 10, 2022

View reviewed changes

pranavkm reviewed Jan 10, 2022

View reviewed changes

Set the body to a null stream after consuming it

df767a7

- This prevents code that would otherwise read the body from doing so after consuming it.

Tratcher reviewed Jan 11, 2022

View reviewed changes

davidfowl added 2 commits January 11, 2022 16:25

PR feedback

191a9cf

- Restore the original stream after the handler executes.

Make sure we can read from the original pipereader when we unwind fro…

a6637e0

…m the call.

davidfowl added 8 commits January 11, 2022 17:58

Support scenarios where nulling out the stream doesn't change the pip…

39152b3

…ereader

Remove support for the raw body as a ROS<byte>

0210b28

Small reverts

f5c305a

Fixed more things

5b03d54

Moar

c1f4063

Reverted more stuff

1cd9735

One more I hope

27c3f48

Fixed the test

9a7f593

BrennanConroy approved these changes Jan 20, 2022

View reviewed changes

davidfowl merged commit 08d1b8a into main Jan 20, 2022

davidfowl deleted the davidfowl/bind-raw-body branch January 20, 2022 18:05

ghost added this to the 7.0-preview1 milestone Jan 20, 2022

davidfowl added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Jan 20, 2022

ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022

Added support for binding the raw request body (dotnet#39388)

7054c18

- Added support for Stream, and PipeReader - Added tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for binding the raw request body #39388

Added support for binding the raw request body #39388

davidfowl commented Jan 9, 2022 •

edited

Loading

JamesNK Jan 9, 2022

davidfowl Jan 9, 2022 •

edited

Loading

davidfowl Jan 9, 2022

halter73 Jan 10, 2022

davidfowl Jan 10, 2022

halter73 Jan 11, 2022 •

edited

Loading

davidfowl Jan 11, 2022 •

edited

Loading

pranavkm Jan 10, 2022

davidfowl Jan 10, 2022

pranavkm Jan 10, 2022

davidfowl Jan 12, 2022 •

edited

Loading

pranavkm Jan 10, 2022

davidfowl Jan 10, 2022

pranavkm Jan 10, 2022

davidfowl Jan 10, 2022

pranavkm Jan 10, 2022

pranavkm Jan 10, 2022

davidfowl Jan 10, 2022

Tratcher Jan 11, 2022

pranavkm Jan 11, 2022

halter73 Jan 11, 2022

davidfowl Jan 11, 2022

davidfowl Jan 11, 2022

davidfowl Jan 11, 2022 •

edited

Loading

halter73 Jan 11, 2022

davidfowl Jan 11, 2022 •

edited

Loading

Tratcher Jan 11, 2022

davidfowl Jan 12, 2022

davidfowl commented Jan 20, 2022

BrennanConroy left a comment

davidfowl commented Jan 20, 2022

	if (_internalPipeReader == null \|\|
	!ReferenceEquals(_streamInstanceWhenWrapped, _context.Request.Body))
	{
	_streamInstanceWhenWrapped = _context.Request.Body;
	_internalPipeReader = PipeReader.Create(_context.Request.Body);

	_context.Response.OnCompleted((self) =>
	{
	((PipeReader)self).Complete();
	return Task.CompletedTask;
	}, _internalPipeReader);
	}

	return _internalPipeReader;

	Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter.");
	Debug.Assert(factoryContext.RequestBodyParameter is null, "factoryContext.RequestBodyParameter is null for a body parameter.");

		public bool AllowEmptyRequestBody { get; set; }
		public bool IsRawRequestBody => RequestBodyParameter?.ParameterType == typeof(ReadOnlySequence<byte>);

Added support for binding the raw request body #39388

Added support for binding the raw request body #39388

Conversation

davidfowl commented Jan 9, 2022 • edited Loading

Choose a reason for hiding this comment

davidfowl Jan 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

davidfowl Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jan 20, 2022

BrennanConroy left a comment

Choose a reason for hiding this comment

davidfowl commented Jan 20, 2022

davidfowl commented Jan 9, 2022 •

edited

Loading

davidfowl Jan 9, 2022 •

edited

Loading

halter73 Jan 11, 2022 •

edited

Loading

davidfowl Jan 11, 2022 •

edited

Loading

davidfowl Jan 12, 2022 •

edited

Loading

davidfowl Jan 11, 2022 •

edited

Loading

davidfowl Jan 11, 2022 •

edited

Loading