Consider behavior when a client disconnects

[rynowak]

Currently today it's possible that you'll get I/O exceptions due to a client disconnect, but it's pretty hard to reason about exactly when that will happen and when it won't due to buffering.

These exceptions can be troublesome in production because you didn't do anything wrong to cause them, and can't do anything to handle or prevent them. Now they are popping up in logs, and you have to try to reason about what's an actual bug vs an I/O exception due to disconnection.

---

The proposal is that we provide a cancellation token for client disconnection that you can pass if you want to know about cancellation, and that the server by default would just ignore your output once a socket is closed.

Rough idea is that this token would be part of the connection feature, and not surfaced through HttpContext because it's so advanced.

[benaadams]

Related aspnet/WebSockets#63

[Tratcher]

There's already a CT that notifies you of client disconnects: httpContext.RequestAborted. You could just check it between writes, or at least between sections.

Maybe you want something like this? https://msdn.microsoft.com/en-us/library/system.net.httplistener.ignorewriteexceptions(v=vs.110).aspx, but on a per request basis?

[clrjunkie]

There's already a CT that notifies you of client disconnects: httpContext.RequestAborted. You could just check it between writes, or at least between sections.

I assume you'll still get the I/O exception if the connection aborts after checking IsCancellationRequested = false, during write.

Maybe you want something like this? https://msdn.microsoft.com/en-us/library/system.net.httplistener.ignorewriteexceptions(v=vs.110).aspx, but on a per request basis?

👍

[Tratcher]

Offline design notes:

• The response Stream Write/Async will no longer throw IOExceptions, they will be logged as debug with the other connection related events.
• It will honor CancellationTokens passed into it and throw OpeartionCancelledException or TaskCancelledException (?) and abort the response. Note the token is only honored for the duration of the write operation, so you get the exception if the token starts cancelled or if there is backpressure in the write-behind buffer and the and the token becomes cancelled. Most small responses will fit entirely within the write-behind buffer.
• Apps that want to detect disconnects can continue to do so using the RequestAborted cancellation token.
• These can be combine to achieve the fail-writes-on-disconnect behavior: await context.Response.WriteAsync(..., context.RequestAborted);

[benaadams]

@Tratcher will try to get some of these in the Timeouts change as I have to detect different conditions for the tests - so will try have it comply.

[benaadams]

or if there is backpressure in the write-behind buffer and the and the token becomes cancelled.

This is a tricky one as different writes can have different tokens. Should they carry their individual tokens with them; throw cancelled if cancelled and log if not? (e.g. you can have a mix of things - if there are several outstanding writes)

Or just store the last cancellation token and only cancel the last write? (Log the others)

[Tratcher]

The token only applies for the duration of the WriteAsync Task, which should complete once the data is copied to the write-behind-buffer, correct?

[benaadams]

Not if you overflow the "considered completed" buffer, when you will get a non-completed task back:

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/SocketOutput.cs#L149-L150

Once the function returns (when it has been copied), you can write more and get more non-completed tasks back; if you aren't awaiting straight away (bad behaviour - though not an error)

They will then complete in order:

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/SocketOutput.cs#L317-L334

Could store the cancellation tokens in addition to the tcs and cancel those that have been cancelled and log those that haven't? This would be for covering the multiple uncompleted tasks; using different cancellation tokens corner case.

[benaadams]

I'm interpreting this as writes should only throw/cancel for writes that have been requested to have the ability be cancelled. Is that correct?

So if you passed CancellationToken.None in for some and RequestAborted for others; only the RequestAborted ones should throw/cancel.

[clrjunkie]

@Tratcher what is the due date for responding to this?

[halter73]

@clrjunkie Due date? Do you disagree with the design? If so, the sooner you raise your objections the more easily we can change direction. No guarantees of course.

[clrjunkie]

I'm still not in that position as I'm juggling between several things. Just wanted to know what timeframe I have to get up to speed as this feels to me tricky enough to justify a group discussion.

[clrjunkie]

Anyway I suggest looking into what httplistener does for this case and see if there are any take aways.

[Tratcher]

@benaadams Yes. Each CT is paired with just the one Task from it's call to WriteAsync. However, Any CT may cause the response to abort.

@clrjunkie HttpListener pre-dates CancellationTokens and Tasks, so there's not much to compare to there.

[clrjunkie]

@Tratcher yes I know that.. I was thinking mor

[clrjunkie]

@Tratcher ...thinking more about any special handling when it swallows the exception as to the connection state..I think its worth taking a peek and see if it just does a catch or something more than that.

[Tratcher]

Catch and abort the request/response/connection.

[clrjunkie]

and what constitutes an unsuccessful write? any return code other than success or eof ?
does it remember this state to skip syscalls for possible successive write attempts by the user?

[Tratcher]

Sure, there's no reason to attempt writes on aborted connections.

[clrjunkie]

Great.