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

Open
Tratcher opened this issue Feb 16, 2019 · 41 comments

Comments

@Tratcher
Copy link
Member

commented Feb 16, 2019

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

This comment has been minimized.

Copy link

commented Feb 17, 2019

As-per aspnet/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

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

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

@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

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

@GusBeare

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

@GusBeare what's the full stack trace?

@GusBeare

This comment has been minimized.

Copy link

commented Mar 7, 2019

I get the following html in the response.

error.txt

@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

@GusBeare let's take this to #8302

@Kaelum

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

@Kaelum you should asynchronously pre-buffer the request body in this case. That's how MVC handles it:

if (!request.Body.CanSeek && !_options.SuppressInputFormatterBuffering)
{
// XmlSerializer does synchronous reads. In order to avoid blocking on the stream, we asynchronously
// read everything into a buffer, and then seek back to the beginning.
var memoryThreshold = DefaultMemoryThreshold;
if (request.ContentLength.HasValue && request.ContentLength.Value > 0 && request.ContentLength.Value < memoryThreshold)
{
// If the Content-Length is known and is smaller than the default buffer size, use it.
memoryThreshold = (int)request.ContentLength.Value;
}
readStream = new FileBufferingReadStream(request.Body, memoryThreshold);
await readStream.DrainAsync(CancellationToken.None);
readStream.Seek(0L, SeekOrigin.Begin);
}

@Kaelum

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented May 30, 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:

It's there AFAIK.

@Kaelum

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

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

@FreyJim

This comment has been minimized.

Copy link

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?

@YetaWF

This comment has been minimized.

Copy link

commented Oct 3, 2019

Sure thing. Already did that. After spending too much time researching this. This was not mentioned on https://docs.microsoft.com/en-us/aspnet/core/migration/22-to-30

We're not all asp.net core fan bois and spend our waking moments following the development. We have real jobs where we have to solve business problems. So something like this just wastes our time.

Of course the solution is always easy, once you know the solution. And since you know how the "enable the setting" again, it would have been nice to show us HOW (for us mere mortals who are not as enlightened). As in:

public void ConfigureServices(IServiceCollection services) {
// If using Kestrel:
services.Configure(options =>
{
options.AllowSynchronousIO = true;
});
// If using IIS:
services.Configure(options =>
{
options.AllowSynchronousIO = true;
});
....

@manigandham

This comment has been minimized.

Copy link

commented Oct 3, 2019

@YetaWF

This change is listed on that very page under the Kestrel section:
https://docs.microsoft.com/en-us/aspnet/core/migration/22-to-30#allowsynchronousio-disabled

Here's docs about that specific setting:
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel?view=aspnetcore-3.0#synchronous-io

Easiest way is in the host builder setup when adding Kestrel:

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureAppConfiguration(builder => builder.AddEnvironmentVariables())
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder
                .UseKestrel(options => options.AllowSynchronousIO = true)
                .UseStartup<Startup>();
        });
@YetaWF

This comment has been minimized.

Copy link

commented Oct 3, 2019

I stand corrected. But like I said, not a fan boi, so I skimmed over that not seeing the implications. I have real work to do...

@manigandham

This comment has been minimized.

Copy link

commented Oct 3, 2019

@YetaWF

Please stop with the silly "fan boi" comments. We all have real work to do.

Reading the docs is expected when moving up to a new major release with breaking changes (and this was just a change in defaults). You can always ask for help too.

@YetaWF

This comment has been minimized.

Copy link

commented Oct 3, 2019

BTW love that "Breaking changes will happen in a new release". I have Windows API code written in 1994 that still works on Windows 10 (that's 25 years ago). So it doesn't have to be that way apparently.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

This is a good change because it'll make developers think twice before doing blocking IO which can cause thread pool starvation in many applications. This is just the first step, expect to see more in this area (analyzers etc). There should be action taken on either the library side or the app side to opt into this bad behavior. Before it was subtle and would result in unreliable applications, now it'll throw and you have to turn on the "I don't care" flag to work around it. Now that flag can be used to audit suspicious applications doing sync IO.

@YetaWF

This comment has been minimized.

Copy link

commented Oct 5, 2019

Indeed, it's a good feature. I actually love that it does this. BUT it kind of flew by under my radar, which caused my frustration.
And yes, if you're developing new code, this is a great way to insure you're not doing something stupid. But if you have a substantial code base that you just want to upgrade from 2.x to 3.0, this will most certainly bite you in the a**.
If you're an overworked (& underpaid) developer, you'll definitely skim over the Kestrel async mention in the migration doc (the Borg are about perfection, I'm about getting things done), and then you'll find out that things don't work.

Now understand that I'm very mindful of sync/async I/O, BUT I use third party stuff I simply have no control over. And that's what failed. For example, System.ServiceModel.Syndication Rss20FeedFormatter WriteTo caused an error. It's synchronous. (I think that's some MS code).
Also some code that writes ZIP files had issues, Both instances, rarely used features in my sites.
That's when I realized what was going on and of course found the local and global "switches" for AllowSynchronousIO.
I also ran across some issue (there was apparently even one in an asp.net core 3 preview) where the amount of data being written was causing an issue. So even at 100% code coverage testing, you still might not see the issue until you have large data I/O.

But why would I EVER want AllowSynchronousIO disabled in PRODUCTION??? That is the default now. That means if I do something stupid (sync I/O) in my code now I crash 100% of the time. Prior to 3.0 I crashed only under high server load (thread starvation). Improvement? I think not.

Yes it's a great feature to audit my code. But I don't want to AUDIT my code in PRODUCTION.
And introducing a "breaking" change shouldn't be about "Hey, your code will now crash for sure". Breaking changes used to be about "your code won't compile any more". Changes that I painstakingly have to verify by 100% code coverage and edge-case testing are not simply "breaking" changes. They're "fu**-you" changes. LOL. (Apologies, I couldn't find a better description).

Obviously now that I know all this, it's a simple matter to adjust my code (audit in debug, don't audit in production). But everyone with an existing non-trivial codebase upgrading to 3.0 will have to go through this "learning" process. Let's say 100,000 developers @ 2 hours each. aspnet core 3.0 has just wasted a shi*-ton of development time. (not quite a crap-ton, but there's always 4.0).

I would love to see more features like this. They're really helpful in finding issues with my code.
But please don't force them down my throat in PRODUCTION. Better yet, let me log such errors in production, to my log file. (THIS is really something I could use). But don't force me to crash, or force me to find all these issues in development.

Unfortunately, the way it's set up now, most people will simply turn AllowSynchronousIO off once they hit the first issue, and that defeats the purpose.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

I get it, we cause pain and I'm sorry. The workaround is quite simple and I hope you didn't lose too much sleep finding it😄. Hopefully, we can do a bit more static analysis next time with analyzers to catch a few more things at build time.

I also get that it's hard to test every code path before deploying to production but this was a major version upgrade after a year of development with some significant features added.

That said, I'm sorry again it caused you pain. As you find these issues in other libraries, let us know and let library authors know. They are bugs that need to be fixed and this change will hopefully cause the masses to influence blocking code 😄 (at least that's my hope).

Unfortunately, the way it's set up now, most people will simply turn AllowSynchronousIO off once they hit the first issue, and that defeats the purpose.

When they hit the issue, it's the first thing I'll ask 😉.

@YetaWF

This comment has been minimized.

Copy link

commented Oct 5, 2019

The workaround is actually counter productive. It just hides the problem. I don't think that's helpful.
Letting me log these errors in production, to my log file, would we 100% more helpful. The way it is now, I have no choice but to enable sync in production (I'll never now what problems I actually hit) and disable sync in development (and hope to hit the problems that may exist). We all know I won't hit 100% of the problems in development. Reality says so.

But keep up the good work, keep the analyzers and code checks coming. I'll make them work somehow. Spelling out the actual implications of some new defaults explicitly will help too.

TL/DR: But if at all possible, give me some interface so I can LOG stuff that is detrimental to my app, like sync I/O, rather than making the app crash. We're all on the same side. We want to make web apps that work.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

Not really there are other options like buffering (which we ourselves had to implement as a result of this feature for things like JSON.NET).

That’s good feedback.

@jhudsoncedaron

This comment has been minimized.

Copy link

commented Oct 7, 2019

Got here, same error message. We built our application server to stream out of blocking objects because it is much faster than running up and down the stack many times per request. Setting AllowSynchronousIO does work, but has to be set for both IIS and Kestrel because the hosting platform can change.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

Why don’t you buffer the request then?

@jhudsoncedaron

This comment has been minimized.

Copy link

commented Oct 7, 2019

@davidfowl : Because this way the caller can begin to consume it before it's even finished coming out of the DB.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

@davidfowl : Because this way the caller can begin to consume it before it's even finished coming out of the DB.

Ah so you prefer to block a thread pool thread than buffer? How big is the payload?

@jhudsoncedaron

This comment has been minimized.

Copy link

commented Oct 7, 2019

@davidfowl : A few MB. I prefer to spend the resource I have (a MB of RAM per request for the blocked thread) over a resource I don't have (another CPU core).

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

@jhudsoncedaron Just be aware that i'll affect your scale if you have too many of those operations happening at the same time. It might seem like a good tradeoff until all of your threads are blocked and you can't do any further operations.

@MaddoxDevelopment

This comment has been minimized.

Copy link

commented Oct 8, 2019

This also errors if you are using Newtonsoft Json. I attempted to make the switch to the new json api's, but there was too much of a change needed for it to work properly, so I'll have to stick with syncronous IO enabled for now.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

@MaddoxDevelopment This is what our JSON.NET InputFormatter does

// JSON.Net does synchronous reads. In order to avoid blocking on the stream, we asynchronously
// read everything into a buffer, and then seek back to the beginning.
var memoryThreshold = DefaultMemoryThreshold;
var contentLength = request.ContentLength.GetValueOrDefault();
if (contentLength > 0 && contentLength < memoryThreshold)
{
// If the Content-Length is known and is smaller than the default buffer size, use it.
memoryThreshold = (int)contentLength;
}
readStream = new FileBufferingReadStream(request.Body, memoryThreshold);
await readStream.DrainAsync(CancellationToken.None);
readStream.Seek(0L, SeekOrigin.Begin);

@MaddoxDevelopment

This comment has been minimized.

Copy link

commented Oct 8, 2019

@MaddoxDevelopment This is what our JSON.NET InputFormatter does

// JSON.Net does synchronous reads. In order to avoid blocking on the stream, we asynchronously
// read everything into a buffer, and then seek back to the beginning.
var memoryThreshold = DefaultMemoryThreshold;
var contentLength = request.ContentLength.GetValueOrDefault();
if (contentLength > 0 && contentLength < memoryThreshold)
{
// If the Content-Length is known and is smaller than the default buffer size, use it.
memoryThreshold = (int)contentLength;
}
readStream = new FileBufferingReadStream(request.Body, memoryThreshold);
await readStream.DrainAsync(CancellationToken.None);
readStream.Seek(0L, SeekOrigin.Begin);

Interesting. I was getting the synchronous IO error right after I added

services.AddMvc()
    .AddNewtonsoftJson();

It seems that it is working fine for most of my endpoints except for this particular one.

https://d.pr/i/u1oirh

The user variable is just a regular C# object, so not sure why it would be throwing.

@jhudsoncedaron

This comment has been minimized.

Copy link

commented Oct 10, 2019

@davidfowl : The task scheduler fails catastrophically when the computer runs out of memory, and there's no way around it. So we had to buy memory instead of CPU. We're currently running with the task scheduler allowed to allocate unlimited threads to avoid thread starvation.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

@MaddoxDevelopment if you have a repro then send me the details with steps, that shouldn't happen.

@jhudsoncedaron Most things fail when the computer runs out of memory and cpu.

@jhudsoncedaron

This comment has been minimized.

Copy link

commented Oct 10, 2019

@davidfowl : I'm just accustomed to throw OutOfMemoryException() rather than deadlock in native code.

@pranavkm

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

@MaddoxDevelopment do you use request.EnableBuffering() in your app?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.