Make StartAsync not throw if we haven't started the response #8199

jkotalik · 2019-03-05T01:12:24Z

One scenario this doesn't support is calling StartAsync after calling GetMemory and using the memory immediately after. @halter73 and I deemed it too difficult to fix well and is an incredibly niche scenario.

public async Task AppFunc(HttpContext context)
{
    var memory = context.Response.BodyPipe.GetMemory();
    await context.Response.StartAsync();
    // Try using the memory here, you'll get an ODE.
    // Advance will also 
}

I added a check that would see if Advance was called immediately after calling StartAsync and throw if so.

jkotalik · 2019-03-05T03:07:42Z

@anurse test discovery issue?

[xUnit.net 00:00:05.78] Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests: Exception thrown during theory discovery on 'Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.RedisEndToEndTests.HubConnectionCanSendAndReceiveMessages'; falling back to single test case.
System.InvalidOperationException: No process is associated with this object.
   at System.Diagnostics.Process.EnsureState(State state)
   at System.Diagnostics.Process.EnsureState(State state)
   at System.Diagnostics.Process.GetWaitState()
   at System.Diagnostics.Process.WaitForExitCore(Int32 milliseconds)
   at System.Diagnostics.Process.WaitForExit()
   at Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.Docker.RunProcessAndWait(String fileName, String arguments, String prefix, ILogger logger, TimeSpan timeout, String& output) in /_/src/SignalR/server/StackExchangeRedis/test/Docker.cs:line 163
   at Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.Docker.RunCommand(String commandAndArguments, String prefix, ILogger logger, String& output) in /_/src/SignalR/server/StackExchangeRedis/test/Docker.cs:line 135
   at Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.Docker.RunCommand(String commandAndArguments, String prefix, String& output) in /_/src/SignalR/server/StackExchangeRedis/test/Docker.cs:line 131
   at Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.Docker.Create() in /_/src/SignalR/server/StackExchangeRedis/test/Docker.cs:line 44
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at System.Lazy`1.get_Value()
   at Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.Docker.get_Default() in /_/src/SignalR/server/StackExchangeRedis/test/Docker.cs:line 23
   at Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.SkipIfDockerNotPresentAttribute.CheckDocker() in /_/src/SignalR/server/StackExchangeRedis/test/SkipIfDockerNotPresentAttribute.cs:line 17
   at Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.SkipIfDockerNotPresentAttribute.get_IsMet() in /_/src/SignalR/server/StackExchangeRedis/test/SkipIfDockerNotPresentAttribute.cs:line 12
   at Microsoft.AspNetCore.Testing.xunit.TestMethodExtensions.EvaluateSkipConditions(ITestMethod testMethod)
   at Microsoft.Extensions.Logging.Testing.LoggedConditionalTheoryDiscoverer.CreateTestCasesForDataRow(ITestFrameworkDiscoveryOptions discoveryOptions, ITestMethod testMethod, IAttributeInfo theoryAttribute, Object[] dataRow)
   at Xunit.Sdk.TheoryDiscoverer.Discover(ITestFrameworkDiscoveryOptions discoveryOptions, ITestMethod testMethod, IAttributeInfo theoryAttribute) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\TheoryDiscoverer.cs:line 238

BrennanConroy · 2019-03-05T03:14:21Z

Didn't realize this was failing builds, should be fixed with #8201

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs

analogrelay · 2019-03-05T16:21:25Z

/azp run AspNetCore-helix-test

azure-pipelines · 2019-03-05T16:21:38Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotalik · 2019-03-05T17:33:21Z

@JamesNK and @JunTaoLuo are you okay with the restrictions on StartAsync that I listed above? After calling StartAsync, don't use the memory that was obtained before calling StartAsync.

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs

Tratcher · 2019-03-05T18:07:56Z

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

@@ -1066,7 +1072,7 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)
                {
                    _keepAlive = false;
                }
-                else if (appCompleted || !_canWriteResponseBody)
+                else if ((appCompleted || !_canWriteResponseBody) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders


if appCompleted && _hasAdvanced then we can set ContentLength to a specific value and avoid chunking.

So I was thinking about doing this in this PR but wasn't a fan. You would need to peek into the number of advanced bytes inside of the Http1OutputProducer which isn't clean. I'd prefer to do it in a separate pr.

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs

JunTaoLuo · 2019-03-05T19:49:53Z

@jkotalik Yes we are fine with that restriction, We currently only call GetSpan after calling StartAsync. We plan to remove the StartAsync calls once this PR is merged so the case of calling GetSpan/GetMemory before StartAsync will not occur.

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs

src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedResponseTests.cs

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs

JamesNK · 2019-03-05T23:23:41Z

@JamesNK and @JunTaoLuo are you okay with the restrictions on StartAsync that I listed above? After calling StartAsync, don't use the memory that was obtained before calling StartAsync.

I'm pretty sure that is fine. I'll take a closer look tomorrow.

jkotalik · 2019-03-05T23:34:16Z

@JamesNK @JunTaoLuo the only thing I'm concerned about is how you mentioned that you wanted to avoid calling StartAsync to have buffering for free so you can modify headers later. It may cause issues if you call StartAsync later, putting you in a weird state.

JamesNK · 2019-03-06T03:31:14Z

We wouldn't call StartAsync while in the middle of writing to Memory<byte>. The scenario we have is writing to the span/memory and calling Advance, then later calling StartAsync.

JunTaoLuo · 2019-03-06T03:42:46Z

then later calling StartAsync.
@JamesNK I think you are thinking of WriteResponseHeadersAsyncCore which calls FlushAsync: https://github.com/grpc/grpc-dotnet/blob/master/src/Grpc.AspNetCore.Server/Internal/HttpContextServerCallContext.cs#L201.

Or did you mean something else?

halter73 · 2019-03-06T22:51:06Z

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

+        // If we are writing with GetMemory/Advance before calling StartAsync, assume we can write and throw away contents if we can't.
+        private bool _canWriteResponseBody = true;
+        private bool _hasAdvanced;
+        private bool _isLeasedMemoryInvalid = true;


Should _hasAdvanced and _isLeasedMemoryInvalid be reset between requests?

Is there a regression test that will fail if we forgot to reset?

Ya caught me.

jkotalik · 2019-03-06T23:40:01Z

🆙 📅

halter73 · 2019-03-06T23:49:36Z

src/Servers/Kestrel/shared/test/TestApp.cs

@@ -49,7 +49,6 @@ public static async Task EchoAppPipeWriter(HttpContext httpContext)
            if (buffer.Length > 0)
            {
                await request.Body.ReadUntilEndAsync(buffer).DefaultTimeout();
-                await response.StartAsync();


I don't think we should have unrelated tests hit the inefficient code path. Tests like ChunkGetMemoryAndWriteWithoutStart should be sufficient.

halter73 · 2019-03-06T23:50:20Z

src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedResponseTests.cs

@@ -534,6 +579,8 @@ public async Task ChunksWithGetMemoryWithInitialFlushWorks()
            {
                var response = httpContext.Response;

+                await response.StartAsync();
+


The old version of this test would still pass, right?

halter73 · 2019-03-06T23:55:55Z

src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedResponseTests.cs

+                await response.StartAsync();
+
+                var memory = response.BodyWriter.GetMemory();
+                length.Value = memory.Length - 1;


FYI: if length was a normal int, this test would still work since the closure would box it.

halter73 · 2019-03-06T23:58:56Z

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs

@@ -3088,7 +3174,7 @@ public async Task ContentLengthWithGetMemoryWorks()
            {
                var response = httpContext.Response;
                response.ContentLength = 12;
-                await response.StartAsync();
+                await Task.CompletedTask;


halter73 · 2019-03-07T00:01:05Z

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs

@@ -2681,7 +2727,6 @@ public async Task WriteAsync_GetMemoryLargeWriteBeforeFirstFlush()
            await InitializeConnectionAsync(async httpContext =>
            {
                var response = httpContext.Response;
-                await response.StartAsync();


If the test isn't explicitly about omitting StartAsync, and calling StartAsync is more efficient, call StartAsync.

halter73

Pending a few nits. I'd like a test verifying _isLeasedMemoryInvalid is reset for example.

jkotalik · 2019-03-07T16:58:43Z

Talked offline, there is actually a good amount of missed scenarios here, especially with resetting state in the Http1OutputProducer. I'll take a look at how to refactor this to avoid this issue in the future.

jkotalik · 2019-03-07T21:28:03Z

@halter73 I added a reset method to HttpOutputProducer. All other ways weren't clean.

src/Servers/Kestrel/test/InMemory.FunctionalTests/ResponseTests.cs

jkotalik requested review from halter73, davidfowl and Tratcher March 5, 2019 01:12

jkotalik assigned jkotalik and unassigned jkotalik Mar 5, 2019

davidfowl reviewed Mar 5, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs Outdated Show resolved Hide resolved

davidfowl reviewed Mar 5, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs Outdated Show resolved Hide resolved

davidfowl reviewed Mar 5, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs Outdated Show resolved Hide resolved

davidfowl reviewed Mar 5, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs Outdated Show resolved Hide resolved

Eilon added the area-servers label Mar 5, 2019

Tratcher reviewed Mar 5, 2019

View reviewed changes