SocketTransport: FIN handling

[tmds]

If a client sends a request and then sends a FIN to indicate it won't send anymore data, this should

• not be treated as an exception
• not cause the server to stop sending a reply.

[davidfowl]

not be treated as an exception

It's an exception because we want application code to fail reading the body. Though we're adding a new pipe for the body reading so we can split app and transport behavior if we found that valuable.

not cause the server to stop sending a reply.

We could make this code only stop the input if the output ended, today it does both things (which happens either way but it's a responsibility question).

Either way, when a fin is received, kestrel will complete the output so the net effect will be the same.

[tmds]

It is valid for an http client to do this:

socket.write(request);
socket.shutdown(SHUT_WR);
response = socket.read();

This won't work well with the Socket Transport. The libuv Transport handles this fine.

[davidfowl]

You may not get the response though. When you send the FIN Kestrel will kill the output potentially before the client gets a chance to read the response. Before we make the sockets transport the default, we'll make sure it has equivalent behavior to the libuv transport.

[davidfowl]

Sent a PR to fix it.

[davidfowl]

The exception case is interesting because libuv doesn't always close with an exception it seems but I think that was an assumption we made.

/cc @CesarBS @halter73

[tmds]

The libuv transport does the proper thing.
When the client sends a FIN, it completes the input without an exception.
When the reading is stopped due to output completing, it completes the input with an exception.

[davidfowl]

When the reading is stopped due to output completing, it completes the input with an exception.

How did you verify that? When does the read callback get triggered with a failure?

[tmds]

I didn't verify it, but I assume this is working code.

Input.Complete(new TaskCanceledException("The request was aborted"));

[davidfowl]

I guess this is only no null for a random UV error or a connection reset.

KestrelHttpServer/src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/LibuvConnection.cs

Line 183 in c2f15fc

Input.Complete(error);

[tmds]

I guess this is only no null for a random UV error or a connection reset.

Yes, error == null when client sends a FIN.
There are two more lines where Input.Complete is called and in those cases an exception is passed.

[davidfowl]

Yep, you're right. I've fixed the issues in #1782

[halter73]

This isn't a problem with just the socket transport. The libuv transport calls IConnectionContext.Abort() for any FIN. Abort() not only immediately poisons the response stream and trips the request aborted token, but it also calls LifetimeControl.End(ProduceEndType.SocketDisconnect).

@davidfowl You didn't forget all the conversations we had about FINs aborting active requests did you? At least we decided to wait until 2.0 to change the behavior.

@CesarBS had to change our test clients to delay the call to Socket.Shutdown(SocketShutdown.Send); until after receiving the full response just to get our test passing in is "Handle client FIN as request abort" #1218 PR.

From the outset, I have been very apprehensive about the change to treat FINs and RSTs the similarly. But @CesarBS verified that nginx truncates responses too if a FIN is received, and I don't think anyone has found a "real" client that will send a FIN prior to receiving a full response.

@tmds do you have an example of an actual HTTP client that does the following?

socket.write(request);
socket.shutdown(SHUT_WR);
response = socket.read();

If so, it's certainly not too late to change the behavior back even if we have to go back to the drawing board and create a new "ClientDisconnected" token with different semantics than the "RequestAborted" token so an EventSource server app can easily observe client disconnection without having to wait for an RST.

[davidfowl]

We can't change th behavior back (at the very least we need o trigger th cancellation token regardless). I don't think there's any new problem, we're just aligning transport behavior as that's what the bug was about.

[tmds]

@halter73 I don't have an example of a client doing that. If nginx treats those FINs as abortive, then it's safe to assume there are no real clients that send FINs before receiving the response.

[halter73]

If a client sends a request and then sends a FIN to indicate it won't send anymore data, this should

• not be treated as an exception
• not cause the server to stop sending a reply.

Kestrel both treats a FIN as an exception and causes the server to stop sending a reply since a FIN immediately triggers the disposal of the UvStreamHandle.

[tmds]

@halter73 I had missed libuv was doing this. From the referenced threads, I now also understand why FINs are treated as abortive.

[davidfowl]

I'm still normalizing the behavior between the 2 transports.