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

[Announcement] AllowSynchronousIO disabled in all servers #7644

Closed
Tratcher opened this issue Feb 16, 2019 · 67 comments
Closed

[Announcement] AllowSynchronousIO disabled in all servers #7644

Tratcher opened this issue Feb 16, 2019 · 67 comments
Labels
breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Milestone

Comments

@Tratcher
Copy link
Member

AllowSynchronousIO is a option in each server that enables or disables sync IO APIs like HttpReqeuest.Body.Read, HttpResponse.Body.Write, Stream.Flush, etc.. These APIs have long been a source of thread starvation and application hangs. Starting in 3.0.0-preview3 these are disabled by default.

Affected servers:

  • Kestrel
  • HttpSys
  • IIS in-process
  • TestServer

Expect errors similar to:

  • Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true instead.
  • Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead.
  • Synchronous operations are disallowed. Call FlushAsync or set AllowSynchronousIO to true instead.

Each server has a AllowSynchronousIO option that controls this behavior and the default for all of them is now false.

The behavior can also be overridden on a per request basis as a temporary mitigation.

var syncIOFeature = HttpContext.Features.Get<IHttpBodyControlFeature>();
if (syncIOFeature != null)
{
    syncIOFeature.AllowSynchronousIO = true;
}

If you have trouble with TextWriters or other streams calling sync APIs in Dispose, call the new DisposeAsync API instead.

@kieronlanning
Copy link

kieronlanning commented Feb 17, 2019

As-per dotnet/AspNetCore.Docs#10983, will this be back ported to 2.x's feature set?

Currently setting this to false in 2.2 breaks response compression.

@BrennanConroy BrennanConroy changed the title [Annoucement] AllowSynchronousIO disabled in all servers [Announcement] AllowSynchronousIO disabled in all servers Feb 17, 2019
@davidfowl
Copy link
Member

No, it won't be back ported. It's a massive breaking change.

@Tratcher
Copy link
Member Author

Note response compression was fixed in 3.0 using the new DisposeAsync API.

@GusBeare
Copy link

GusBeare commented Mar 7, 2019

I have been working on porting an angularJS app to .Net Core and just upgraded to Preview 3. After upgrade some of the client GET requests now blow with this error:

InvalidOperationException: Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead.

I don't get it because by default the angular requests are async and why do just some of them break?

I am using VS 2019 Preview and will be hosting IIS in process.

Any ideas?

@Tratcher
Copy link
Member Author

Tratcher commented Mar 7, 2019

@GusBeare what's the full stack trace?

@GusBeare
Copy link

GusBeare commented Mar 7, 2019

I get the following html in the response.

error.txt

@Tratcher
Copy link
Member Author

Tratcher commented Mar 7, 2019

@GusBeare let's take this to #8302

@Kaelum
Copy link

Kaelum commented May 17, 2019

This change in .NET 3.0 Preview 5 breaks the usage of XmlSerializer.Deserialize(Stream). Do we now need to change AllowSynchronousIO to true? Or should we be performing serialization differently? As stated by others, this is a significant break change over all previous versions of ASP.NET.

Reference dotnet/coreclr#24643

@Tratcher
Copy link
Member Author

@Kaelum
Copy link

Kaelum commented May 29, 2019

@Tratcher the FileBufferingReadStream constructor above doesn't appear to be in the 3.0 Preview 5 libraries. Also, I have concerns that using FileBufferingReadStream might be too slow, if it is actually caching to the filesystem. We need to process a minimum of 12,000 requests/sec, which we do by keeping everything in memory. I tried using the BufferedReadStream object, but under the covers it is doing everything synchronously as well. This is what I have tried:

Works, but I have concerns with caching to the file system, and the constructor in your example doesn't exist:

using (FileBufferingReadStream readstream = new FileBufferingReadStream(request.Body, memoryThreshold, null, @"D:\Temp\", ArrayPool<byte>.Shared))
{
	await readstream.DrainAsync(CancellationToken.None);
	readstream.Seek(0L, SeekOrigin.Begin);

	XmlSerializer xmlSerializer = new XmlSerializer(typeof(XmlRequestBcap));
	xmlRequestBcap = xmlSerializer.Deserialize(readstream) as XmlRequestBcap;
}

Works, but probably non-performant:

using (StreamReader streamReader = new StreamReader(request.Body, Encoding.UTF8))
using (StringReader stringReader = new StringReader(await streamReader.ReadToEndAsync()))
{
	XmlSerializer xmlSerializer = new XmlSerializer(typeof(XmlRequestBcap));
	xmlRequestBcap = xmlSerializer.Deserialize(stringReader) as XmlRequestBcap;
}

Does not work, executes synchronously:

using (BufferedReadStream readstream = new BufferedReadStream(request.Body, memoryThreshold))
{
	if (!await readstream.EnsureBufferedAsync(CancellationToken.None))
	{
		throw new InvalidOperationException("Request body is empty.");
	}

	XmlSerializer xmlSerializer = new XmlSerializer(typeof(XmlRequestBcap));
	xmlRequestBcap = xmlSerializer.Deserialize(readstream) as XmlRequestBcap;
}

Variation of above that works, adding a MemoryStream, but is it safe and performant:

using (BufferedReadStream readstream = new BufferedReadStream(request.Body, memoryThreshold))
{
	if (!await readstream.EnsureBufferedAsync(CancellationToken.None))
	{
		throw new InvalidOperationException("Request body is empty.");
	}

	using (MemoryStream memoryStream = new MemoryStream(readstream.BufferedData.Array, readstream.BufferedData.Offset, readstream.BufferedData.Count, false))
	{
		XmlSerializer xmlSerializer = new XmlSerializer(typeof(XmlRequestBcap));
		xmlRequestBcap = xmlSerializer.Deserialize(memoryStream) as XmlRequestBcap;
	}
}

If FileBufferingReadStream is the most performant, and recommended for our case, what happened to the constructor that I see in your source reference? It is definitely not in the published 3.0 preview.

Thanks!

@davidfowl
Copy link
Member

@Tratcher the FileBufferingReadStream constructor above doesn't appear to be in the 3.0 Preview 5 libraries. Also, I have concerns that using FileBufferingReadStream might be too slow, if it is actually caching to the filesystem. We need to process a minimum of 12,000 requests/sec, which we do by keeping everything in memory. I tried using the BufferedReadStream object, but under the covers it is doing everything synchronously as well. This is what I have tried:

It's there AFAIK.

@Kaelum
Copy link

Kaelum commented May 30, 2019

@davidfowl & @Tratcher I just verified that it is not in the SDK 3.0.100-preview5-011568 build. The referenced Microsoft.AspNetCore.WebUtilities assembly is pointing to:

C:\Program Files\dotnet\packs\Microsoft.AspNetCore.App.Ref\3.0.0-preview5-19227-01\ref\netcoreapp3.0\Microsoft.AspNetCore.WebUtilities.dll

When I disassemble that assembly, I can very definitely see only 4 constructors, with the 2 parameter constructor missing.

@Tratcher
Copy link
Member Author

Here's the MVC PR: #9806
I think that was a preview6 change.

@FreyJim
Copy link

FreyJim commented Aug 12, 2019

@Tratcher
I have the same issue by using streams with moq framework. I changed already inside the moq setup to call the async methods and also added the section to the startup class.
services.Configure<TestServer>(options => { options.AllowSynchronousIO = });

But I still get the same error message:
System.InvalidOperationException: Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true.

Do I have to do something special by using the TestServer?

@Tratcher
Copy link
Member Author

What's the stack trace?

@Tratcher
Copy link
Member Author

How are you creating TestServer in your test? The old pattern was to do new TestServer(webhostbuilder); which wouldn't use DI to create an instance so services.Configure<TestServer>(options => { options.AllowSynchronousIO = }); wouldn't apply. If you are newing it up directly then you can directly set the option as well.

The new pattern looks like this and would work with DI options:
https://github.com/aspnet/AspNetCore/blob/8c02467b4a218df3b1b0a69bceb50f5b64f482b1/src/Hosting/TestHost/test/TestServerTests.cs#L53-L62

workgroupengineering pushed a commit to workgroupengineering/Dotmim.Sync that referenced this issue Jun 15, 2020
@danstur
Copy link

danstur commented Jul 12, 2020

@pranavkm We are hitting this issue with EnableBuffering active. A simplified scenario:

// Inside Middleware
var buffer = new byte[4000];
context.Request.EnableBuffering();
context.Request.Body.Position = 0;
var readBytes = await context.Request.Body.ReadAsync(buffer, 0, buffer.Length);
context.Request.Body.Position = 0;

results sometimes, but non-deterministically (afaics) in an InvalidOperationException with Synchronous IO APIs are disabled, see AllowSynchronousIO and the following stack trace:

at int Microsoft.AspNetCore.Server.HttpSys.RequestStream.Read(out byte[] buffer, int offset, int size)
   at int Microsoft.AspNetCore.WebUtilities.FileBufferingReadStream.Read(byte[] buffer, int offset, int count)
   at int Microsoft.AspNetCore.WebUtilities.HttpRequestStreamReader.ReadIntoBuffer()
   at int Microsoft.AspNetCore.WebUtilities.HttpRequestStreamReader.Read(char[] buffer, int index, int count)
   at int Newtonsoft.Json.JsonTextReader.ReadData(bool append, int charsRequired)
   at bool Newtonsoft.Json.JsonTextReader.ParseObject()
   at void Newtonsoft.Json.JsonReader.Skip()
   at void Newtonsoft.Json.Serialization.JsonSerializerInternalReader.HandleError(JsonReader reader, bool readPastError, int initialDepth)
   at object Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, string id)
   at object Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, object existingValue)
   at object Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, object existingValue)
   at object Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, bool checkAdditionalContent)
   at async Task<InputFormatterResult> Microsoft.AspNetCore.Mvc.Formatters.NewtonsoftJsonInputFormatter.ReadRequestBodyAsync(InputFormatterContext context, Encoding encoding)
   at async Task Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BodyModelBinder.BindModelAsync(ModelBindingContext bindingContext)
   at async Task<ModelBindingResult> Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, object value)
   at void Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider+<>c__DisplayClass0_0+<<CreateBinderDelegate>g__Bind|0>d.MoveNext()
   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()+Awaited(?)
   at async Task Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextExceptionFilterAsync()+Awaited(?)

are we using the API incorrectly or should we create a new issue for this?

@Tratcher
Copy link
Member Author

You're only doing a single ReadAsync on the body, you need to read until the end of the body to ensure it's fully buffered. Read in a loop until it returns 0.

@pha3z
Copy link

pha3z commented Sep 23, 2020

If I understand this change correctly, I appreciate it. But I am not clear on how to pre-buffer the request body to access it in other parts of asp.net.

I implemented my own HMAC Authentication Handler that I add with services.AddAuthentication(). I want to confirm that the body has not been tampered with in transit, so I include request body as input to encryption token on the client and then do the same thing on the server to verify its integrity. The problem is that I can't read Request.Body in the server Authentication Handler method since reading the stream directly can interfere with the rest of the pipeline.

It would be nice if there was a simple switch that you can use in Startup to automatically buffer every request before sending anything down the pipeline. That way Authentication Handlers or whatever else you want to use have access to the entire Request Body before handlers are ever executed. Is this possible?

The only examples I find when I search for pre-buffering Request.Body talk about doing it inside the controller method. Clearly, that's not helpful because anything in Controller happens AFTER Authentication Handlers or other initiating handlers.

@Tratcher
Copy link
Member Author

Tratcher commented Sep 24, 2020

HttpRequest.EnableBuffering is the simplest mechanism.

public static void EnableBuffering(this HttpRequest request)

The example above mostly covered it, except that it needs to read to the end of the body.

It would be nice if there was a simple switch that you can use in Startup to automatically buffer every request before sending anything down the pipeline. That way Authentication Handlers or whatever else you want to use have access to the entire Request Body before handlers are ever executed. Is this possible?

Yes, via middleware using the example above.

Note we don't recommend buffering globally or make it any easier because it has side-effects like no longer being able to override request body size limits per endpoint.

@davidfowl
Copy link
Member

@Tratcher lets make sure to get this documented.

@Tratcher
Copy link
Member Author

Filed dotnet/AspNetCore.Docs#20001

@matthiaslischka
Copy link

Is there a way to enable it for specific calls? We do use a third party component that does not support async calls and it feels wrong to set the whole application to AllowSynchronousIO just for this one call.

@Tratcher
Copy link
Member Author

@matthiaslischka Yes:

The behavior can also be overridden on a per request basis as a temporary mitigation.

var syncIOFeature = HttpContext.Features.Get<IHttpBodyControlFeature>();
if (syncIOFeature != null)
{
    syncIOFeature.AllowSynchronousIO = true;
}

@davidfowl
Copy link
Member

We need guidance for this, you have 2 options:

  • Buffering asynchronously in memory or on disk (or a hybrid)
  • Allow synchronous IO and hope for the best.... (like what @Tratcher mentions above)

@matthiaslischka
Copy link

We have this scenario where we create a zip file for some very small files and stream it directly to the HttpContext response stream.
The library we use does not support asynchronous calls.

Without drastically changing the implementation we now can

  1. allow synchronous IO operations for this specific call or
  2. use a memory stream and then at the end copy the memory stream to the HttpContext response stream

I'm not sure which one is better/cleaner.
Not sure if (2) is a good solution or just a dirty workaround. Does it improve the IO load?

@Tratcher
Copy link
Member Author

@matthiaslischka If the files are small then buffering to a memory stream is better. It avoids accidentally blocking threads with IO, and it will probably perform better because it will aggregate small writes from the zip library.

@ghost
Copy link

ghost commented Jan 9, 2021

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

@ghost
Copy link

ghost commented Mar 11, 2021

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

@ghost ghost closed this as completed Mar 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

No branches or pull requests