Revert the output pipe in the DuplexStreamPipeAdapter

[davidfowl]

• Don't complete the connection pipe in Http2FrameWriter
• Replace the output pipe only
• Updated tests

Fixes #11560

[halter73]

Given that both LibuvConnection and SocketConnection will not complete their PipeReaders with an error unless it was completely unexpected, it's tempting to make this a critical log. That's unless we're worried about something like SslStream throwing an Exception in the middle of WriteAsync despite there being no transport issue.

Maybe that is something we should be worried about... I'm just extra worried about swallowing errors now.

[jkotalik]

I'm going to leave it at error for now. I do agree that swallowing errors is a concern, but what else should we do. Debug.Assert?

[halter73]

David already passed in a logger here. Critical logs are the new, much-improved Debug.Assert. Any critical log should cause the majority of our tests to fail.

[halter73]

We should probably use the defaults here like we did before to reduce thrashing:

https://github.com/aspnet/AspNetCore/blob/release/3.0-preview6/src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs#L67-L76

        internal PipeOptions AdaptedOutputPipeOptions => new PipeOptions
        (
            pool: MemoryPool,
            readerScheduler: PipeScheduler.Inline,
            writerScheduler: PipeScheduler.Inline,
            pauseWriterThreshold: _context.ServiceContext.ServerOptions.Limits.MaxResponseBufferSize ?? 0,
            resumeWriterThreshold: _context.ServiceContext.ServerOptions.Limits.MaxResponseBufferSize ?? 0,
            useSynchronizationContext: false,
            minimumSegmentSize: MemoryPool.GetMinimumSegmentSize()
        );

[halter73]

Please fix the pauseWriterThreshold and resumeWriterThreshold before merging though.

[halter73]

👍

[halter73]

Maybe trying out the lower thresholds are OK for a preview. It does seem a little unecsary to try out new options now if we're removing it completely in the next preview though.

[davidfowl]

Maybe trying out the lower thresholds are OK for a preview. It does seem a little unecsary to try out new options now if we're removing it completely in the next preview though.

I'd rather leave it as it so its as passthrough as possible. Also the fact that it is temporary and the fact that those options aren't readily available (they need to be flowed from the UseHttps call) is just more churn. If you do feel strongly about changing those limits back to the Kestrel limits, I'll do it.

[halter73]

If you do feel strongly about changing those limits back to the Kestrel limits, I'll do it.

I'm fine with the way it is since it's temporary. Otherwise, I'd at least ask for benchmarks.

[davidfowl]

I'm fine with the way it is since it's temporary. Otherwise, I'd at least ask for benchmarks.

Maybe it's reasonable to leave the defaults then.

[davidfowl]

I'm fine with the way it is since it's temporary. Otherwise, I'd at least ask for benchmarks.

I just realized though, you'll very likely never hit those limits since we always consume the entire buffer and the scheduling is inline. It means call to write will yield the await, write to the Stream then call Advance). It'll only matter when we're experiencing back pressure from the transport itself (which is the effect we want, complete passthrough).

[halter73]

I just realized though, you'll very likely never hit those limits since we always consume the entire buffer and the scheduling is inline. It means call to write will yield the await, write to the Stream then call Advance). It'll only matter when we're experiencing back pressure from the transport itself (which is the effect we want, complete passthrough).

That has always been my assumption. I talked to @jkotalik about exactly this earlier today. That's why I'm not too worried. I like validating my assumptions though.