Call PipeWriter.CompleteAsync instead of PipeWriter.Complete

[halter73]

Now that PipeWriter.CompleteAsync() has been added (https://github.com/dotnet/corefx/issues/39241), we should call PipeWriter.CompleteAsync() wherever we call PipeWriter.Complete() today and wouldn't want to to sync-over-async IO. PipeWriter.Complete() can potentially do sync-over-async IO given a non-default PipeWriter. StreamPipeWriter is the big existing example of this since it will flush the stream in PipeWriter.Complete()/CompleteAsync().

@anurse @davidfowl @jkotalik

[analogrelay]

Necessary for 3.0 or could this be done in 3.1?

[jkotalik]

Really should be in 3.0.

[analogrelay]

Should be isn't necessary for though ;). I'm not saying I'll punt it out if we say it's not necessary but I'd like to know what it means to not take this in 3.0

[halter73]

I'd like to know what it means to not take this in 3.0

It means there could be sync-over-async flushes if you use the BodyWriter and don't fully flush it. I am fine with punting this in 3.1.

[davidfowl]

It means there could be sync-over-async flushes if you use the BodyWriter and don't fully flush it. I am fine with punting this in 3.1.

Not really right, if you call Complete after not having flushed data, in the servers we'll just flush once the app func runs. We should look at override CompleteAsync to return the right task to the caller (if @Tratcher didn't already do that in his PR).

[halter73]

I'm not sure I understand the point you're trying to make @davidfowl.

Complete does always "flush" after the app func runs, but even StreamPipeWriter will elide the flush to the inner stream if all the data has already been flushed. DefaultPipeWriter would similarly have no reason to block on IO if all the data has been flushed by the app.

[halter73]

Are we thinking about flushing trailers? That's somewhat interesting. The workaround would be to call CompleteAsync() yourself, but I can see how that's a real problem.

[halter73]

BTW, calling PipeWriter.Complete() on Kestrel's HttpResponsePipeWriter will never block, so even the trailers shouldn't be a huge deal. Instead, HttpResponsePipeWriter.Complete() calls CompleteAsync() ands stores the task in a field so it can be awaited later in the request processing loop before we start processing the next request.

https://github.com/aspnet/AspNetCore/blob/98abd9e256f5a204d706d91793da950680f41061/src/Servers/Kestrel/Core/src/Internal/Http/HttpResponsePipeWriter.cs#L40

That said, a surgical fix to call PipeWriter.CompleteAsync() instead of PipeWriter.Complete() in the OnCompleted() callbacks added by response body wrappers would be very trivial, and probably worthwhile.

[davidfowl]

Are we thinking about flushing trailers? That's somewhat interesting. The workaround would be to call CompleteAsync() yourself, but I can see how that's a real problem.

Yes.

BTW, calling PipeWriter.Complete() on Kestrel's HttpResponsePipeWriter will never block, so even the trailers shouldn't be a huge deal. Instead, HttpResponsePipeWriter.Complete() calls CompleteAsync() ands stores the task in a field so it can be awaited later in the request processing loop before we start processing the next request.

Sure that's what I was getting at, the caller can't observe it inline but it doesn't cause any problems. Seems like we could override CompleteAsync and expose the task to the caller.

That said, a surgical fix to call PipeWriter.CompleteAsync() instead of PipeWriter.Complete() in the OnCompleted() callbacks added by response body wrappers would be very trivial, and probably worthwhile.

We do that already.

[Tratcher]

Except flushing in OnCompleted() fails because the response has already been cleaned up. The only thing calling CompleteAsync in OnCompleted accomplishes is returning buffers, and that is always synchronous, right?

[davidfowl]

Except flushing in OnCompleted() fails because the response has already been cleaned up. The only thing calling CompleteAsync in OnCompleted accomplishes is returning buffers, and that is always synchronous, right?

Pretty much. But we can't know what other implementations will do.