-
Notifications
You must be signed in to change notification settings - Fork 524
Add feature to optionally disallow synchronous IO #1919
Conversation
@@ -31,6 +34,7 @@ public override long Position | |||
set => throw new NotSupportedException(); | |||
} | |||
|
|||
// REVIEW: Should Flush() throw? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, we discussed this yesterday and there are some reasons it shouldn't. StreamWriter
always calls Flush on Dispose https://github.com/dotnet/corefx/blob/6dd451f51451a7d0ceea6104b51bd17005e9a0e6/src/System.Runtime.Extensions/src/System/IO/StreamWriter.cs#L188.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kestrel's flush needs to throw for all the same reasons write is throwing, sync over async. HttpSys's flush no-ops unless you haven't sent the headers, so throwing could be conditional there but that would be even more confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means people can't use StreamWriter
over the body even if they wrote everything async. We'll need guidance for this. Maybe you're forced to use "leaveOpen" in this case since you should never dispose the underlying stream anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is our recommendation for response streams anyways. E.g. we already make Dispose no-op, it's the servers job to clean up the stream.
That said, StreamWriter calls Flush even for LeaveOpen:
https://github.com/dotnet/corefx/blob/6dd451f51451a7d0ceea6104b51bd17005e9a0e6/src/System.Runtime.Extensions/src/System/IO/StreamWriter.cs#L184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we need to tell people to not dispose the StreamWriter at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StreamWriter.Dispose calls Stream.Flush no matter what. You can prevent StreamWriter.Dispose from calling Stream.Write by either calling StreamWriter.FlushAsync immediately before or using "leaveOpen".
@@ -330,6 +331,7 @@ public void Reset() | |||
|
|||
HasStartedConsumingRequestBody = false; | |||
MaxRequestBodySize = ServerOptions.Limits.MaxRequestBodySize; | |||
AllowSynchronousIO = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialize from KestrelServerOptions?
@@ -36,6 +36,11 @@ public class KestrelServerOptions | |||
public SchedulingMode ApplicationSchedulingMode { get; set; } = SchedulingMode.Default; | |||
|
|||
/// <summary> | |||
/// Gets or sets a value that controls whether synchronous IO is allowed for the <see cref="Http.Features.IHttpRequestFeature.Body"/> and <see cref="Http.Features.IHttpResponseFeature.Body"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpContext.Request/Reponse.Body
@@ -36,6 +37,11 @@ public class KestrelServerOptions | |||
public SchedulingMode ApplicationSchedulingMode { get; set; } = SchedulingMode.Default; | |||
|
|||
/// <summary> | |||
/// Gets or sets a value that controls whether synchronous IO is allowed for the <see cref="HttpContext.Request"/> and <see cref="HttpContext.Response"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the doc comments to reference HttpContext instead of IHttpRequest/ResponseFeature. The problem is if I reference HttpContext.Request/Response.Body, I get the following error:
KestrelServerOptions.cs(40,101): error CS1574: XML comment has cref attribute 'Body' that could not be resolved [D:\dev\aspnet\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel.Core\Microsoft.AspNetCore.Server.Kestrel.Core.csproj]
KestrelServerOptions.cs(40,144): error CS1574: XML comment has cref attribute 'Body' that could not be resolved [D:\dev\aspnet\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel.Core\Microsoft.AspNetCore.Server.Kestrel.Core.csproj]
Any ideas why that is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it can't navigate multiple properties deep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. At the same time, we want to reference HttpContext in the doc comment, right? I'll just leave it as is then.
7ed4590
to
4a93cd6
Compare
⬆️ 📅 |
@@ -305,7 +306,7 @@ public void InitializeStreams(MessageBody messageBody) | |||
{ | |||
if (_frameStreams == null) | |||
{ | |||
_frameStreams = new Streams(this); | |||
_frameStreams = new Streams(this, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use keyword args.
return ReadAsync(buffer, offset, count).Result; | ||
if (!_bodyControl.AllowSynchronousIO) | ||
{ | ||
throw new InvalidOperationException("Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true instead."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move string to CoreStrings.
@@ -58,6 +61,11 @@ public override void SetLength(long value) | |||
|
|||
public override void Write(byte[] buffer, int offset, int count) | |||
{ | |||
if (!_bodyControl.AllowSynchronousIO) | |||
{ | |||
throw new InvalidOperationException("Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoreStrings
stream.StartAcceptingReads(body); | ||
|
||
input.Add("Hello"); | ||
|
||
var buffer = new byte[1024]; | ||
|
||
var count = stream.Read(buffer, 0, buffer.Length); | ||
var count = await stream.ReadAsync(buffer, 0, buffer.Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test (and similar-named ones) are specifically testing sync reads. Pass a mock feature that allows sync I/O.
@@ -510,12 +510,11 @@ public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIg | |||
var testLogger = new TestApplicationErrorLogger(); | |||
var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) }; | |||
|
|||
using (var server = new TestServer(httpContext => | |||
using (var server = new TestServer(async httpContext => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is specifically checking behavior on sync I/O (note there are tests with Write in the name, and other tests with WriteAsync). Enable sync I/O here.
@@ -967,12 +965,11 @@ public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIg | |||
{ | |||
var flushed = new SemaphoreSlim(0, 1); | |||
|
|||
using (var server = new TestServer(httpContext => | |||
using (var server = new TestServer(async httpContext => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable sync I/O here - test is specific to sync I/O.
@@ -1248,7 +1245,7 @@ public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIg | |||
[Fact] | |||
public async Task FirstWriteVerifiedAfterOnStarting() | |||
{ | |||
using (var server = new TestServer(httpContext => | |||
using (var server = new TestServer(async httpContext => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test wants to use sync I/O.
@@ -1289,7 +1285,7 @@ public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIg | |||
[Fact] | |||
public async Task SubsequentWriteVerifiedAfterOnStarting() | |||
{ | |||
using (var server = new TestServer(httpContext => | |||
using (var server = new TestServer(async httpContext => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test wants to use sync I/O.
@CesarBS 🆙 📆ed to address your feedback |
stream.StartAcceptingReads(body); | ||
|
||
input.Add("Hello"); | ||
|
||
var buffer = new byte[1024]; | ||
Assert.Equal(0, stream.Read(buffer, 0, buffer.Length)); | ||
Assert.Equal(0, await stream.ReadAsync(buffer, 0, buffer.Length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should be sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
#1521