Delay in client-initiated abort for event stream request

[nphmuller]

This issue was part of #1103 (see Issue 1), but has been moved to a different issue report to improve trackability. I have tested this on tag 1.0.0, rel/1.0.1 and 3104110 (the commit after #1103 has been marked as completed)

Say I have a controller that sends a 'text/event-stream' response, which writes (Response.WriteAsync()) a string every second. When the client aborts the request (like clicking on the 'Stop' button in chrome), the request in the controller should be aborted immediately. This means that HttpContext.RequestAborted should be set to cancelled immediately. This however doesn't happen. There always seems to be a delay of a couple of WriteAsync() calls

In the sample below, 2 things get written to the console: Starting response {i} and Ended response {i}. Now, when I fire the request and immediately abort the request from the browser, the console has the following output:

info: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
Request starting HTTP/1.1 GET http://localhost:5000/
Started response 1 of 20
Ended response 1 of 20
Started response 2 of 20
Ended response 2 of 20
Started response 3 of 20
info: Microsoft.AspNetCore.Server.Kestrel[14]
Connection id "0HKV17V8EGG77" communication error
Microsoft.AspNetCore.Server.Kestrel.Internal.Networking.UvException: Error -4081 ECANCELED operation canceled
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[2]
Request finished in 2133.1317ms 200 text/event-stream

The cancellation isn't noticed until the start of the 3rd loop. This always happens and has no connection to the Task.Delay() value, because a delay of 1000 ms and 10.000 ms produce the same result.

After some research it seems the delay comes all the way from libuv.
For every WriteAsync() call in the sample, libuv calls back to UvWriteCb() in UvWriteReq.cs, with a pointer to the request (UvWriteReq) and a status code describing how the write has been handled. Only after the 3rd WriteAsync() call libuv reports status code -4081 (ECANCELED), which immediately triggers the abort flow in Kestrel.

On a side-note: this also happens with other content-types, but I've used a text/event-stream type in this issue as a most likely use-case.

Sample code:

using System;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;

namespace SampleApp
{
    public class Startup
    {
        public static void Main(string[] args)
        {
            new WebHostBuilder().UseKestrel().UseUrls("http://localhost:5000").UseStartup<Startup>().Build().Run();
        }

        public void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)
        {
            loggerFactory.AddConsole(LogLevel.Information);
            app.Run(async ctx =>
            {
                ctx.Response.Headers.Add("Content-Type", "text/event-stream");
                var ct = ctx.RequestAborted;
                for (int i = 0; i < 20; i++)
                {
                    Console.WriteLine($"Started response {i + 1} of 20");
                    try
                    {
                        await Task.Delay(1000, ct);
                        await ctx.Response.WriteAsync($"data: Hello ({DateTime.Now})\r\r");
                    }
                    catch (TaskCanceledException)
                    {
                        break;
                    }
                    Console.WriteLine($"Ended response {i + 1} of 20");
                }
            });
        }
    }
}

[Tratcher]

If you trace the TCP session do you see the browser close the connection first with a FIN or RST?
/cc @davidfowl

[davidfowl]

This is the same problem, it's probably a FIN

[nphmuller]

@davidfowl Yeah, you're right.
FIN, ACK, followed by RST, ACK.

[SteveSandersonMS]

@muratg Can this issue be added to the 1.1.0-preview1 milestone like its predecessor #1103 was? I ask because anyone trying to use the JavaScriptServices-based templates will not be able to use HTTPS in development mode until this is fixed.

[muratg]

@SteveSandersonMS preview1 will be closed soon, tentatively adding it to this milestone. If not, we'll look into it at 1.1.0.

[halter73]

@CesarBS We are going to want to see what other servers and clients do.

The suggested fix, tripping the RequestAborted token when the client sends a FIN, may not be appropriate since the TCP connection is still in a CLOSE_WAIT state where it's legal to write data from the server to the client (e.g. a Response). Tripping RequestAborted would break StaticFiles if there are any clients that send a FIN immediately after sending the request or any time prior to receiving the complete response. This is because the StaticFiles middleware passes the RequestAborted token to all calls to Response.Body.WriteAsync.

There's been some speculation that all HTTP clients will wait until receiving the last response in its entirety before sending a FIN. If that's the case I'm fine with tripping the RequestAborted token then. However, this isn't mandated by the HTTP spec, and I don't think this has been well tested yet. The RequestAborted will be tripped today (but will be "delayed") if the client rejects any packets with a RST.

The alternative is to provide a seperate token that's tripped when the connection enters the CLOSED_WAIT state. AFAICT this new token would only be used by those implementing an SSE style transport.

[SteveSandersonMS]

@halter73 Is fixing this issue the only way that the ObjectDisposedException on HTTPS connections is going to be fixed, or could the ObjectDisposedException be fixed in some simpler way? Just asking because the delayed-abort-notification issue is much less important than the ObjectDisposedException from the point of view of integrating with Webpack dev middleware.

[cesarblum]

@SteveSandersonMS The ObjectDisposedException was fixed in #1127.

[muratg]

It would be good to test a few different clients to ensure we're not regressing any clients.

We also need to check IIS static file handler behavior too (does it kill the response upon receiving a FIN?).

@CesarBS will be doing a bit more investigation before we decide if this is in 1.1 or 1.2.

[cesarblum]

As discussed, I tested IIS with a client that requests a large file and immediately sends a FIN. IIS still sends the entire file.

It's hard to come across HTTP/1.0 clients, but I did check curl and it waits for a FIN from the server before sending it's own.

I think the best approach to this would be to provide a separate token, as suggested by @halter73.

[Tratcher]

Or we change static files to behave like IIS? It doesn't actually need the RequestAborted token, it just needs an active token so that WriteAsync will throw for send failures, correct? If that's the case it can create its own CTS.

[cesarblum]

So we trip the token on FIN and change StaticFiles to not break? I don't know, the behavior seems weird to me. Why trip a "request aborted" token just because the client sent a FIN? Unless we rename the token.

[Tratcher]

Because there's precedence for it. That's how IIS and ASP.NET 4 behave, and we we know there are existing clients that implement aborts that way, including our own .NET 4.5 HttpClient. Even Kestrel favors sending FINs over RSTs.

[cesarblum]

Can you point me to the docs on that? If we're going to do that I'd like to at least change the token's name. It seems super confusing that a "request aborted" token gets tripped just because the client signaled its done sending the request. Something like "client disconnected" would better reflect this behavior.

The problem of course is that that would break the API. Could we add a proxy property to the token with the new name?

[davidfowl]

We can't change the name and I doubt this will break anyone 😄. We can fix static files as @Tratcher said. We could add another feature interface but that might be overkill here.

[Tratcher]

We realized yesterday that the static files change won't work, it still requires the token to be cancelled.

The missing piece of this discussion is that we have failed to prove even one client would be broken if we changed this behavior (e.g. are there any that preemptively send a FIN?). What we do know is that there are many clients broken without it.
http://stackoverflow.com/questions/39574061/httprequest-not-aborted-cancelled-on-browser-abort-in-asp-net-core-mvc

[cesarblum]

There's an interesting discussion on this topic here: https://lists.w3.org/Archives/Public/ietf-http-wg-old/2001JanApr/0036.html

I'm still reading through it. Also reading Section 6 of RFC 7230 to see if it says anything at all about this (the discussion linked above predates the updated RFC).

[Tratcher]

Additional task: verify the behavior of the same FIN abort scenario when Kestrel is running behind IIS+ANCM.

[cesarblum]

@Tratcher IIS does not foward the client FIN to Kestrel.

[cesarblum]

nginx truncates the response if it receives an eager FIN from the client.

[NinoFloris]

@CesarBS But nginx does immediately forward it right?

[cesarblum]

@NinoFloris Yes. Here's a network trace:

nginx is listening on port 8080, Kestrel on 5000. The browser is using port 5310 to connect to nginx and nginx is using port 5311 to connect to Kestrel.