SocketOutput exception completes all waiting tasks

[benaadams]

Lightens #565
Resolves #450
Resolves #566

[benaadams]

Confirmed this resolves the issue we were experiencing with Ssl (forced sync) and high connection churn; while triggering pending tasks (> maxBytesPreCompleted, websocket streaming) .

Essentially thrashing the server with our worse case situation.

[halter73]

Thanks for doing this. I agree with this approach. This is type of change is what @rynowak and I were considering when he filed the issues at the end of last week.

I think we also want to fail all pending tasks if OnWriteCompleted is called after a SocketDisconnect just to be safe.

Another consideration is how this is going to interact with the per-request IgnoreWriteExceptions property we are likely to add. We still haven't decided whether or not it will be opt in or out, but I'm personally leaning toward opt-in.

This PR also conflicts a little with #562 so one of us is going to have to rebase depending on the order these get in (and we know who controls that 😛). Seriously though, I'm going to try to merge #562 today and the rebase shouldn't be too difficult.

[benaadams]

so one of us is going to have to rebase depending on the order these get in (and we know who controls that 😛

😆

Another consideration is how this is going to interact with the per-request IgnoreWriteExceptions property we are likely to add.

Isn't too hard; though would be an issue if someone was streaming something large but the client had disconnected as if they weren't listening for it they'd just keep writing to null?

[halter73]

We decided not to add an IgnoreWriteExceptions property. We will never throw from SocketOutput.WriteAsync. FrameResponseStream will be modified to honor the CancellationToken that is passed in for async writes.

If you want the IgnoreWriteExceptions = false behavior you will always pass in the IHttpRequestLifetimeFeature.RequestAborted token to your writes. If FrameResponseStream ever throws due to a canceled token it will abort the connection.

We will also plan to log all write failures using LogLevel.Debug correlated with the connection id. The connection id will be made accessible from user code via a new IHttpConnectionFeature.ConnectionId property.

[benaadams]

What about lines new lines 86-89 ---^

[halter73]

Are you talking about if (_lastWriteError != null) throw ...? If so, we'd remove that and just ignore the write while returning a completed task.

[benaadams]

Going to roll this into #485; if that works for you?

[halter73]

That will probably fine. We definitely appreciate the work. Try to separate the changes related to #566 (comment) into their own commit if you can.

[benaadams]

Will split into 2 commits when I know the shape of it

[benaadams]

Reopening. Need to sort out a new test

[benaadams]

@halter73 Added test, probably could do with some comments - but otherwise seem ok?

[cesarblum]

nit: add new line before method

[benaadams]

Made behaviour same between frameworks and added comments to new test

[benaadams]

Should return cancelled task rather than throwing TaskCanceledException?

[halter73]

I don't think we should swallow this error. I think there is a chance that exceptions from callbacks registered with the token will bubble up through this call. I would probably use KestrelTrace.ApplicationError.

[benaadams]

Need to run through this again in morning; though I just think need a fake connection for tests to remove ? in _connection?.Abort();

[benaadams]

@halter73 should be there? Issues with CI on Github though (libuv copy)

[halter73]

@CesarBS should have a fix for the libuv copy issue. He discovered the issue is that the Libuv-Copier is looking for libuv in ~/.dnx/packages instead of the now used ~/.nuget/packages directory.

[halter73]

It seems to me that this is only getting called if the user passes in a canceled token, but that's only one scenario where this is getting called.

We need to complete all writes whenever there is an error, even if we don't wind up cancelling them.

[benaadams]

Hmm... let me run through the logic again

[benaadams]

Fixed, though I have it Completing written ones first on Error, then doing the others. Probably should just do all regardless of counts? Also reworded the commits

[halter73]

I have it Completing written ones first on Error, then doing the others

This has no functional impact vs completing all the writes at once on error, right? If so, that's fine, as long as the code stays simple.

[benaadams]

No, but it looks a bit weird. Let me change it since the CI is failing anyway.

[benaadams]

Resolved, was just a case of moving one line of code - looks better now

[benaadams]

libuv copy grumpy

[halter73]

Note to self: We should add a test where a bunch of pending writes that get completed b/c of a failed write instead of a canceled token before merging.

I can do this myself since this will require some small modifications to MockLibuv (I know _uv_err_name and _uv_strerror will need to be implemented.)

There's also a pretty big chance that we might need to merge a more limited version of this PR that only ensures writes always get completed, and merge the CancellationToken related chances post-RC2.

[benaadams]

Pre-newer CancellationToken changes, needs to only throw if given CancellationToken is cancelled otherwise it should return null.

[benaadams]

Linked #599

[benaadams]

Rebased