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

HttpRequest.BodyReader.TryRead returns false even though request has a body #55615

Closed
1 task done
Xor-el opened this issue May 8, 2024 · 6 comments
Closed
1 task done
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@Xor-el
Copy link

Xor-el commented May 8, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

HttpRequest.BodyReader.TryRead returns false (and ReadResult isn't populated) even though the request has a body and the readResult isn't completed or cancelled.
When using HttpRequest.BodyReader.ReadAsync, the ReadResult response is populated successfully.

Here is a Demo that reproduces the issue.

Expected Behavior

HttpRequest.BodyReader.TryRead should not return false when a request body exists and the readResult isn't cancelled or completed.

Steps To Reproduce

  1. Clone this Repo, build and run.

  2. Fire a POST request to https://localhost:7244/bodyreaderbugdemo/working with the request body below

{
     "name": "Chucky",
     "description": "Child's Play is an American slasher media franchise created by Don Mancini."
}

Notice that the response is a 200 Ok

  1. Now, fire another POST request to https://localhost:7244/bodyreaderbugdemo/buggy with the same request body as above.

Notice that we get a 400 Bad Request

Key Notes

  1. Both endpoints employ filters that shortcircuits if the read request body is null.

  2. The first endpoint uses a filter ExtractRequestBodyWorking which uses HttpRequest.BodyReader.ReadAsync to read request body. This works as expected.

  3. However, the second endpoint uses another filter ExtractRequestBodyBuggy which uses HttpRequest.BodyReader.TryRead to read request body. in this case, HttpRequest.BodyReader.TryRead returns false even though a request body exists.

Exceptions (if any)

No response

.NET Version

8.0.200

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label May 8, 2024
@Xor-el Xor-el changed the title HttpRequest.BodyReader.TryRead returns false even though Request has a body HttpRequest.BodyReader.TryRead returns false even though request has a body May 8, 2024
@Tratcher
Copy link
Member

Tratcher commented May 9, 2024

This is a misunderstanding. TryRead returns false if data isn't immediately available. TryRead or ReadAsync may still return data in the future. The request may have a large body, but it isn't available yet.

I would also suggest trying this without request body buffering, that construct can skew your results since it involves pipe<->stream adapters, you don't have direct access to the server buffers.
https://github.com/dotnet/runtime/blob/52638488c45ef62d1e9273902f57c010c6310f1d/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs#L465-L468

@Xor-el
Copy link
Author

Xor-el commented May 9, 2024

I would also suggest trying this without request body buffering, that construct can skew your results since it involves pipe<->stream adapters, you don't have direct access to the server buffers. https://github.com/dotnet/runtime/blob/52638488c45ef62d1e9273902f57c010c6310f1d/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs#L465-L468

Thanks for replying @Tratcher .
I read some where some months back that Asp.Net Core does not buffer request body by default so if we need to read the body multiple times, we have to enable request.EnableBuffering.

To clear all doubts, I tried your suggestion, I removed request.EnableBuffering and now when trying to read using either ReadAsync or TryRead, they fail with an exception.
I then removed httpRequest.Body.Seek(0, SeekOrigin.Begin); and bodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.Start); and everything seems to work Ok.
does this mean that I do not have to reset the bodyReader in the finally block after reading?

@Tratcher
Copy link
Member

Tratcher commented May 9, 2024

You can put that all back, it was just removed to simplify the scenario. The real fix is to replace TryRead with ReadAsync.

@Xor-el
Copy link
Author

Xor-el commented May 9, 2024

You can put that all back, it was just removed to simplify the scenario. The real fix is to replace TryRead with ReadAsync.

Ok. but say I need to use TryRead, continue instead of break would be the proper approach right?
something like the code below.

        public static T? ReadBody<T>(this HttpRequest httpRequest)
        {
            var bodyReader = httpRequest.BodyReader;

            ReadResult readResult = default;
            
            do
            {
                if (!bodyReader.TryRead(out readResult))
                    continue; // continue instead of breaking here ??

                bodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End);

            } while (!readResult.IsCanceled && !readResult.IsCompleted);

            var buffer = readResult.Buffer;

            if (buffer.IsEmpty)
                return default;

            var reader = new Utf8JsonReader(buffer);

            return JsonSerializer.Deserialize<T>(ref reader, JsonExtensions.JsonOptions);
        }

@Tratcher
Copy link
Member

Tratcher commented May 9, 2024

If TryRead returns false then you need to call ReadAsync next, not stop reading entirely.

@Xor-el
Copy link
Author

Xor-el commented May 11, 2024

If TryRead returns false then you need to call ReadAsync next, not stop reading entirely.

in that case, I will stick to just using HttpRequest.BodyReader.ReadAsync.
Thanks for your time. closing this issue now.

@Xor-el Xor-el closed this as completed May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

No branches or pull requests

2 participants