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

EnableBuffering not working on .Net Core 3.0.100 #14396

Closed
SledgeHammer01 opened this issue Sep 24, 2019 · 31 comments
Closed

EnableBuffering not working on .Net Core 3.0.100 #14396

SledgeHammer01 opened this issue Sep 24, 2019 · 31 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@SledgeHammer01
Copy link

.Net Core 3.0.100

Startup.cs:

		app.Use(next => context =>
		{
			var syncIOFeature = context.Features.Get<IHttpBodyControlFeature>();
			if (syncIOFeature != null)
			{
				syncIOFeature.AllowSynchronousIO = true;
			}

			context.Request.EnableBuffering();
			return next(context);
		});

When I try to access the stream by context.HttpContext.Request.Body later on, it reports that the stream has already been disposed and an exception is thrown.

@SledgeHammer01
Copy link
Author

Also tried:

services.Configure(options =>
{
options.AllowSynchronousIO = true;
});

Same result.

@analogrelay
Copy link
Contributor

Can you post a minimal, runnable, sample project that reproduces the issue. Specifically, it's important to know exactly when in the control flow you are accessing HttpContext.Request.Body.

@analogrelay analogrelay added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 24, 2019
@halter73
Copy link
Member

If you're getting an ObjectDisposedException from HttpContext.Request.Body, that means that the middleware handling the request has already completed.

@SledgeHammer01
Copy link
Author

Hi, some further information:

Startup.cs:

in ConfigureServices(IServiceCollection services)...

		services.Configure<IISServerOptions>(options =>
		{
			options.AllowSynchronousIO = true;
		});

		// set Mvc / Json options

		services.AddMvc(options => options.EnableEndpointRouting = false).AddNewtonsoftJson(options =>

in Configure(IApplicationBuilder app, IWebHostEnvironment env)…

		// enable response buffering

		app.UseResponseBuffering();

		// enable request stream rewinding

		app.Use(next => context =>
		{
			context.Request.EnableBuffering();
			return next(context);
		});

All controllers derive from ControllerBase class which derives from Controller.

In ControllerBase.cs:

public override async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
{
}

I am trying to access the request body in there so I can log the full json. This worked in .Net 1.x and 2.x. In 3.0, I get that the stream has been disposed.

@halter73
Copy link
Member

I get that the stream has been disposed.

Do you have logs and/or exception details including a stack trace indicating where the ObjectDisposedException was thrown from?

@SledgeHammer01
Copy link
Author

The callstack of the exception itself is:

at System.IO.MemoryStream.get_Length()
at Microsoft.AspNetCore.WebUtilities.FileBufferingReadStream.get_Length()

The app call stack is:

yyy.dll!xxx.yyy.Common.ControllerBase.OnActionExecutionAsync(Microsoft.AspNetCore.Mvc.Filters.ActionExecutingContext context, Microsoft.AspNetCore.Mvc.Filters.ActionExecutionDelegate next) Line 46 C#
[External Code]
yyy.dll!xxx.yyy.Startup.Configure.AnonymousMethod__3(Microsoft.AspNetCore.Http.HttpContext context) Line 164 C#
[External Code]

@davidfowl
Copy link
Member

@SledgeHammer01 can you provide a runnable example with all of the code? It's diffcult to figure out what's going on without seeing the entire application. Even better would be to come up with a minimal repro.

@SledgeHammer01
Copy link
Author

SledgeHammer01 commented Sep 25, 2019

I'm trying to put together an example, just cleaning it up... but got a fuller stack trace... will post an example probably tomorrow morning.

System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'FileBufferingReadStream'.
   at Microsoft.AspNetCore.WebUtilities.FileBufferingReadStream.ThrowIfDisposed()
   at Microsoft.AspNetCore.WebUtilities.FileBufferingReadStream.Seek(Int64 offset, SeekOrigin origin)
   at MyCompany.Test.Common.ControllerBase.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next) in C:\Users\xxx\Documents\Visual Studio 2019\Projects\Test\Common\ControllerBase.cs:line 41
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Buffering.ResponseBufferingMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

HEADERS

Accept: application/json
Accept-Encoding: gzip, deflate
Accept-Language: en-US
Cache-Control: no-cache
Connection: Keep-Alive
Content-Length: 103
Content-Type: application/json
Host: localhost:52580
Referer: http://localhost:52580/swagger/index.html
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; Trident/7.0; rv:11.0) like Gecko

@halter73 halter73 added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 25, 2019
@SledgeHammer01
Copy link
Author

Ok, here you go...

Test.zip

When you run, it'll bring up the Swagger page, and try out testMethod2 for example, it'll throw the exception in ControllerBase.cs : OnActionExecutionAsync.

Thanks!

@halter73
Copy link
Member

I added the area-mvc label because it looks like something in MVC is disposing the request stream when it wasn't before. This is likely the regression.

I'm leaving the area-servers label because it might be better for FileBufferingReadStream.Dispose() to no-op like it does for the raw request stream.

@halter73 halter73 removed the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 25, 2019
@SledgeHammer01
Copy link
Author

Is there a work-around? The goal is to be able to log the raw request in a central location.

@davidfowl
Copy link
Member

@pranavkm Did we change anything here that you're aware of?

@pranavkm
Copy link
Contributor

Did we change anything here that you're aware of?

The Json.NET based input formatter buffers the request body to avoid synchronously reading the request body. MVC avoids double buffering if it thinks that the request is already buffered: https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs#L132

@halter73
Copy link
Member

halter73 commented Sep 26, 2019

https://github.com/aspnet/AspNetCore/blob/b44e9c6a24528c5ebec95da60295b4b74f83f8ec/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs#L173-L176

The above seems super suspect. It appears as if NewtonsoftJsonInputFormatter assumes that if readStream is a FileBufferingReadStream, it was created here:

https://github.com/aspnet/AspNetCore/blob/b44e9c6a24528c5ebec95da60295b4b74f83f8ec/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs#L144

But it's possible that readStream is still a reference to HttpContext.Request.Body that just happened to be set to a FileBufferingReadStream before the NewtonsoftJsonInputFormatter ran:

https://github.com/aspnet/AspNetCore/blob/b44e9c6a24528c5ebec95da60295b4b74f83f8ec/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs#L131

@pranavkm
Copy link
Contributor

Yeah, that's the bug. We'll need to set a flag that says that MVC owns the stream. @mkArtakMSFT this is a small enough change we could consider squeezing in for 3.1.

@pranavkm pranavkm added bug This issue describes a behavior which is not expected - a bug. and removed area-servers investigate labels Sep 26, 2019
@pranavkm pranavkm removed their assignment Sep 26, 2019
@mkArtakMSFT
Copy link
Member

Is this a regression in 3.0?

@davidfowl
Copy link
Member

Yes

@mkArtakMSFT mkArtakMSFT added this to the 3.1.0-preview3 milestone Sep 28, 2019
@mkArtakMSFT mkArtakMSFT removed this from the 3.1.0-preview3 milestone Oct 1, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.1.0-preview2 milestone Oct 1, 2019
@keenjus
Copy link

keenjus commented Oct 3, 2019

Is there a work-around? The goal is to be able to log the raw request in a central location.

Remove calls to AddNewtonsoftJson()

@SledgeHammer01
Copy link
Author

@keenjus -- I can't remove that. We use some of the Json.Net hooks to correct the wrong serialization. The new json serializer has the same issues (doesn't maintain correct property order).

@davidfowl
Copy link
Member

There’s no workaround and this should be considered for patching in 3.1

@davidfowl
Copy link
Member

Well I take that back, the workaround is to create a wrapping stream that does nothing when dispose is called (but has to dispose the inner stream at the right time as well)

@SledgeHammer01
Copy link
Author

Hi David, I did actually find a temp work-around... pulling out the body right after the call to EnableBuffering and then rewinding the stream to 0 and not disposing it.

@afshawnlotfi
Copy link

afshawnlotfi commented Oct 7, 2019

@SledgeHammer01 I tried that still getting "" as the response. I'm using F# but the concept is the same:

    let GetRequestBody (context : HttpContext) = 
        task {
            try
                context.Request.EnableBuffering()
                let reader = new StreamReader(context.Request.Body, encoding = Encoding.UTF8, detectEncodingFromByteOrderMarks = false, bufferSize = 1024, leaveOpen = true) 
                let! body = (reader.ReadToEndAsync())
                context.Request.Body.Position <- 0L;

                return Ok body
            with _ ->
                return Error "failed to get http body"
    
        }

@halter73
Copy link
Member

halter73 commented Oct 7, 2019

@afshawnlotfi where is GetRequestBody(context) being called from? Is it possible the request was already read? Did you debug to see if body was an empty string?

@pranavkm
Copy link
Contributor

We've addressed the reported issue as part of fcc20ac. This should be fixed in 3.1-preview2. In the meanwhile, @davidfowl's suggestion here - #14396 (comment) - would be the way to go.

@afshawnlotfi could you please file a separate issue?

@RomanTymchyshyn
Copy link

RomanTymchyshyn commented Oct 25, 2019

Decided to tell my case here. Hope it will help somebody and save time 😃

I ran into a similar issue because I was using .AddNewtonsoftJson and middleware with .EnableBuffering, which resulted in FileBufferingReadStream being disposed in NewtonsoftJsonInputFormatter.

Here is my workaround as suggested by the comment #14396 (comment)

Workaround:
https://repl.it/@RomanTymchyshyn/EnableBufferingPatched

.EnableBufferingPatched extension replaces .EnableBuffering and uses FileBufferingReadStreamNoDispose which will do actual dispose only when called through reference to IDisposable or IDisposableAsync.
I had to add BufferRequest method and drain the request body manually, because if request CanSeek, then NewtonsoftJsonInputFormatter will not drain it and it will result in synchronous reads from original request stream.

Looks hacky, but it works 🤣

Other way to go in my case is to read request before passing it down the pipeline.

I had logging middleware like this:

await context.Request.BufferRequest();

var originalBodyStream = context.Response.Body;

using (var responseBody = new MemoryStream())
{
    context.Response.Body = responseBody;

    try
    {
        await next(context);
    }
    catch (Exception e)
    {
        await LogRequestResponse(context, responseBody, originalBodyStream, e);
        throw;
    }

    await LogRequestResponse(context, responseBody, originalBodyStream);
}

LogRequestResponse tried to read disposed stream.
So, instead of introducing a hacky solution with wrapper stream I can read the request body before passing it to next and then log already fetched body in LogRequestResponse.

@RomanTymchyshyn
Copy link

BTW, is it OK that NewtonsoftJsonInputFormatter will not drain and reset the stream if it CanSeek?
Sees like in 3.1-preview2 if EnableBuffering is used, app will throw sync IO exception instead of closed stream.

@pranavkm
Copy link
Contributor

BTW, is it OK that NewtonsoftJsonInputFormatter will not drain and reset the stream if it CanSeek?

@davidfowl we spoke about this. Think we should just address this in the formatter, it improves the experience for everyone involved.

@davidfowl
Copy link
Member

Auto drain? Yes I agree

@aidbal
Copy link

aidbal commented Nov 6, 2019

Hey guys, as 3.1-preview2 is out, the same issue still occurs. I have enabled httpContext.Request.EnableBuffering(), but I still get "Cannot access a disposed object". Are there more pending changes that are not released yet?

@pranavkm
Copy link
Contributor

pranavkm commented Nov 6, 2019

@aidbal would you mind starting a new issue for this so we can track it correctly?

@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests