Skip to content

Minimal api stream sample #26437

Merged
Rick-Anderson merged 41 commits into
dotnet:mainfrom
sammychinedu2ky:minimal-api-stream-sample-sam
Jul 19, 2022
Merged

Minimal api stream sample #26437
Rick-Anderson merged 41 commits into
dotnet:mainfrom
sammychinedu2ky:minimal-api-stream-sample-sam

Conversation

@sammychinedu2ky
Copy link
Copy Markdown
Contributor

@sammychinedu2ky sammychinedu2ky commented Jul 15, 2022

Added sample code for an issue #26374

Fixes #26346

@sammychinedu2ky sammychinedu2ky changed the title Minimal api stream sample sam Minimal api stream sample Jul 15, 2022
Copy link
Copy Markdown
Contributor

@Rick-Anderson Rick-Anderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor changes

@Rick-Anderson
Copy link
Copy Markdown
Contributor

Rick-Anderson commented Jul 15, 2022

@sammychinedu2ky I did suggestions so you just have to "Add suggestion to batch" and then "Commit"
Then just do a git pull to bring in the changes locally.

Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>
@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

@sammychinedu2ky I did suggestions so you just have to "Add suggestion to batch" and then "Commit" Then just do a git pull to bring in the changes locally.

I've added the changes

sammychinedu2ky and others added 3 commits July 15, 2022 02:41
Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>
Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>
Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>
@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

sammychinedu2ky commented Jul 15, 2022

ooh, I see the logger you added, cool. I have added the changes

@Rick-Anderson
Copy link
Copy Markdown
Contributor

On the other PR, See https://github.com/dotnet/AspNetCore.Docs/blob/main/.gitignore#L15-L16 and check in the solution file. Usually you don't need the sln file so we ignore it.

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

ooh ok let me work on that

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

Please how did you get to see the sln file when it wasn't pushed?

@Rick-Anderson
Copy link
Copy Markdown
Contributor

Please how did you get to see the sln file when it wasn't pushed?

I didn't, I just figured you had one sln for both projects because that's typical.

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

Please how did you get to see the sln file when it wasn't pushed?

I didn't, I just figured you had one sln for both projects because that's typical.

ooh I see 😅

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

So I should work on putting the test in a single project?

@Rick-Anderson Rick-Anderson requested a review from halter73 July 15, 2022 02:01
Co-authored-by: David Fowler <davidfowl@gmail.com>
@davidfowl
Copy link
Copy Markdown
Member

@sammychinedu2ky The code is almost complete, but we need to discuss the hardest part of this PR, the performance and potential security issues:

  • We're buffering the data into memory. The default max request body size on Kestrel is ~28.6 MB. That's massive.
  • We're potentially going on the large object heap if we have a single buffer over 85K (this isn't great).
  • Since we're using a bounded channel (yay!), we're restricting the queue to 100 items at a time. Anything about that will wait on enqueue (do we want to instead drop the request and return a 429 status code?)

To avoid making the sample more complex than it needs to be, a happy medium would be to limit the size of the buffer we use for the memory stream.

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

sammychinedu2ky commented Jul 16, 2022

  • 429

I can use a 429 if trywrite returns false or let me reduce it to 20

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

@sammychinedu2ky The code is almost complete, but we need to discuss the hardest part of this PR, the performance and potential security issues:

  • We're buffering the data into memory. The default max request body size on Kestrel is ~28.6 MB. That's massive.
  • We're potentially going on the large object heap if we have a single buffer over 85K (this isn't great).
  • Since we're using a bounded channel (yay!), we're restricting the queue to 100 items at a time. Anything about that will wait on enqueue (do we want to instead drop the request and return a 429 status code?)

To avoid making the sample more complex than it needs to be, a happy medium would be to limit the size of the buffer we use for the memory stream.

But won't I still need to return a 429 if it exceeds 20?

@davidfowl
Copy link
Copy Markdown
Member

We could do some back of the napkin math here and come up with some numbers that are reasonable and show readers how those calculations affect the limits. Lets say we're willing to use 500MB on this message processing (we're on a budget here). If the max message size is 1MB, we can support up to 500 concurrent requests. We can bound the channel to 500 and use TryWrite to write to the channel, if it fails, we can reject the request with a 429.

Now limiting the request body to 1MB is interesting, as this will still land us in the LOH range. We also don't want to start with a 1MB buffer as that's wasteful for small buffers but we want to resize up to 1MB and then explode for buffers over that size. I'm still thinking about the most reasonable way to accomplish that without exploding people's heads.

The other option is to stay below the LOH size (85K) and allow up to 80K bodies for this endpoint (which would leave a bounded channel around 6,400).

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

We could do some back of the napkin math here and come up with some numbers that are reasonable and show readers how those calculations affect the limits. Lets say we're willing to use 500MB on this message processing (we're on a budget here). If the max message size is 1MB, we can support up to 500 concurrent requests. We can bound the channel to 500 and use TryWrite to write to the channel, if it fails, we can reject the request with a 429.

Now limiting the request body to 1MB is interesting, as this will still land us in the LOH range. We also don't want to start with a 1MB buffer as that's wasteful for small buffers but we want to resize up to 1MB and then explode for buffers over that size. I'm still thinking about the most reasonable way to accomplish that without exploding people's heads.

The other option is to stay below the LOH size (85K) and allow up to 80K bodies for this endpoint (which would leave a bounded channel around 6,400).

ok but the body size for the sample is 63 bytes

Co-authored-by: David Fowler <davidfowl@gmail.com>
@davidfowl
Copy link
Copy Markdown
Member

davidfowl commented Jul 16, 2022

ok but the body size for the sample is 63 bytes

But people copy and paste sample code into applications and will blame Microsoft when there's a security breach 😄. The sample payload if 63 bytes, but an attacker can send any size post (well up to ~28.6 MB by default).

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

sammychinedu2ky commented Jul 16, 2022

ok but the body size for the sample is 63 bytes

But people copy and paste sample code into applications and will blame Microsoft when there's a security breach 😄. The sample payload if 63 bytes, but an attacker can send any size post (well up to ~28.6 MB by default).

You're right 😅

Co-authored-by: David Fowler <davidfowl@gmail.com>
Co-authored-by: David Fowler <davidfowl@gmail.com>
Copy link
Copy Markdown
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks clean @sammychinedu2ky. I hope you had fun with it!

@sammychinedu2ky
Copy link
Copy Markdown
Contributor Author

This looks clean @sammychinedu2ky. I hope you had fun with it!

Yh it was quite insightful 😎and fun as well

@Rick-Anderson Rick-Anderson merged commit 35e1845 into dotnet:main Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.NET 7: Add sample for Minimal API Bind the request body as a Stream or PipeReader

3 participants