Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support binding request body as Stream/ReadOnlySequence<byte> maybe ReadOnlyMemory<byte>/ReadOnlySpan<byte> #38153

Closed
davidfowl opened this issue Nov 7, 2021 · 2 comments · Fixed by #39388
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels cost: M Will take from 3 - 5 days to complete Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing Priority:1 Work that is critical for the release, but we could probably ship without triage-focus Add this label to flag the issue for focus at triage
Milestone

Comments

@davidfowl
Copy link
Member

Is your feature request related to a problem? Please describe.

There's a common pattern where you have a web front end that ingests data and enqueues to into an azure queue (storage or service bus etc) that is then processed by a worker (https://docs.microsoft.com/en-us/azure/architecture/guide/architecture-styles/web-queue-worker). This pattern usually results in a taking an incoming JSON payload, creating a BinaryData from it (this means it's fully buffered) and enqueuing that payload to the queue.

Here's what this might look like in .NET 6:

app.MapPost("v1/feeds", async (QueueClient queueClient, SomeDto dto, CancellationToken cancellationToken) =>
{
    await queueClient.CreateIfNotExistsAsync(cancellationToken: cancellationToken);
    await queueClient.SendMessageAsync(new BinaryData(dto), cancellationToken: cancellationToken);
});

This version has a few costs:

  • We de-serialize the entire JSON payload into the SomeDto object.
  • We serialize the SomeDto object into a single JSON ut8 byte[]
  • If the serialized form is > 85K, it'll end up on the LOH (Large object heap)

Describe the solution you'd like

I would like it to be possible to grab the entire request body as a ReadOnlySequence<byte>, a Stream or a pooled backed Memory<byte>/Span<byte> (though this requires a copy so we should consider this one carefully).

It would enable this:

app.MapPost("v1/feeds", async (QueueClient queueClient, ReadOnlySequence<byte> body, CancellationToken cancellationToken) =>
{
    await queueClient.CreateIfNotExistsAsync(cancellationToken: cancellationToken);
    await queueClient.SendMessageAsync(new BinaryData(body), cancellationToken: cancellationToken);
});

Additional context

  • When injecting the Stream, it would be the same object as HttpRequest.Body
  • We would not buffer the request body by default so after it is read, it wouldn't be rewindable (e.g you can't read the stream multiple times).
  • We would the ReadOnlySequence<byte> would not be usable outside of the handler. Doing so would be dangerous since the underlying memory would be reused outside of this handler. This means fire and forget scenarios should copy the buffer.
  • The ReadOnlyMemory/ReadOnlySpan would be usable outside of the handler since it's a copy of the body.
@davidfowl davidfowl added feature-minimal-actions Controller-like actions for endpoint routing area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Nov 7, 2021
@rafikiassumani-msft rafikiassumani-msft added this to the .NET 7 Planning milestone Nov 15, 2021
@rafikiassumani-msft rafikiassumani-msft added Priority:1 Work that is critical for the release, but we could probably ship without cost: M Will take from 3 - 5 days to complete triage-focus Add this label to flag the issue for focus at triage labels Jan 10, 2022
@BrennanConroy
Copy link
Member

Note: We added support for Stream and PipeReader.
Span<byte>, byte[], and ReadOnlySpan<byte> were found to be unfriendly to use/implement in a clean and safe manner.

@davidfowl
Copy link
Member Author

3 ideas that could help us bring back a buffered body solution:

  • A representation of a lifetime of the buffer (maybe an IMemoryOwner) or something reference counted (😨)
  • A default body max size and an attribute to set the max body size
  • Metadata you can apply to the endpoint to limit the size of the buffered body

@BrennanConroy BrennanConroy added the Docs This issue tracks updating documentation label Feb 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels cost: M Will take from 3 - 5 days to complete Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing Priority:1 Work that is critical for the release, but we could probably ship without triage-focus Add this label to flag the issue for focus at triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants