Implement PipeBody Features and add to HttpContext

[jkotalik]

For #3972.
Add more Stream/Pipe interleaving test to verify stream and pipe can be read from/ written two sequentially.

[davidfowl]

In this case, do we lose the old reference to the pipe? Who is responsible for cleaning it up? Can this leak memory? Does it matter?

[jkotalik]

Tough situation. Someone can keep a reference to the BodyPipe like:

  context.Response.Body = new MemoryStream();
  var canBeDisposed = context.Response.BodyPipe;
  context.Response.Body = Stream.Null;
  var nonDisposed = context.Response.BodyPipe;

Should we dispose canBeDisposed in this situation? I don't think we would dispose the stream in this situation, so I don't think we should call dispose here.

[halter73]

Considering we're implicitly constructing the StreamPipeWriter, we should dispose it. I don't think we can expect developers to know they might have dispose the old response body StreamPipeWriter just because they replaced the response body Stream.

In this case it's not trivial to track who constructed the _pipeWriter, but we should try. It's also probably a good idea to fail any I/O being done on the old _pipeWriter instead of closing in gracefully.

[jkotalik]

In this case it's not trivial to track who constructed the _pipeWriter, but we should try.

I think the tracking needs to exist inside of the feature. I'll take a stab at something.

It's also probably a good idea to fail any I/O being done on the old _pipeWriter instead of closing in gracefully.

You are effectively asking for us to call Abort on the underlying stream, correct?

[jkotalik]

Also, what happens if someone wraps the BodyPipe with another PipeReader/Writer. For example, if we were to reimplement ResponseCompression with Pipes, we would do:

ctx.Response.BodyPipe = new ResponseCompressionPipeWriter(ctx.Response.BodyPipe);

which would fail because the BodyPipe is now disposed.

[jkotalik]

We will now call HttpResponse.RegisterForDispose() for all adapters that we set.

[davidfowl]

@jkotalik @halter73 @Tratcher servers have to re-implement this logic https://github.com/aspnet/AspNetCore/pull/6394/files#diff-f773336ba05e72446c3f058e83ca3bc4R30 yeah? If a server implements these features it has to handle the wrapping and unwrapping manually. These features will only be used by Http.Sys server? Is that correct? (Assuming we do native pipes in both IIS and Kestrel).

[Tratcher]

@davidfowl Kestrel, IIS, and TestServer need to implement this feature but I expect they'd do so in reverse. Pipe would be the default and Stream the fallback. And yes, the implementation here will primarily be used for Http.Sys.

[halter73]

Why throw when setting a null request body pipe? We don't throw when setting a null request body stream do we?

[jkotalik]

I've bounced back and forth between this a few times. There are a few things we need to think about:

When setting the BodyPipe, we should update Body accordingly. When someone sets BodyPipe to null, either we can throw or set both Body and BodyPipe to null. Today, when someone sets Body to null, it remains null.

So I think I agree with you.

[Tratcher]

Body should never be set to null. Stream.Null maybe, but not null. No we don't throw for that at the moment but in hindsight we should have.

I don't see a scenario for setting pipe to null either.

[halter73]

Considering we're implicitly constructing the StreamPipeWriter, we should dispose it. I don't think we can expect developers to know they might have dispose the old response body StreamPipeWriter just because they replaced the response body Stream.

In this case it's not trivial to track who constructed the _pipeWriter, but we should try. It's also probably a good idea to fail any I/O being done on the old _pipeWriter instead of closing in gracefully.

[Tratcher]

Offline discussion on Dispose: You can't. Consider the decorator example for something like response compression. It wraps the inner stream with its own that does the transformation and reassigns the response body field. Then it undoes that wrapping at the end of the response. The same style wrapping should be possible for pipes, so the original value must be kept alive.

[halter73]

Offline discussion on Dispose: You can't.

You can and you must if you don't want to leak pooled memory.

When we construct a StreamPipeReader/Writer, we've decided that the body Stream is the source of truth. This only wraps StreamPipeReader/Writers, not arbitrary pipes, so there's no risk of this replacing a transformative pipe.

[jkotalik]

@halter73 see above. Thoughts on adding it to RegisterForDispose.

[Tratcher]

@halter73 we worked out a cleaner solution: register the pipe for dispose on creation, not on replacement.

[jkotalik]

We have to call RegisterForDispose here because by default the DefaultHttpContext doesn't call Uninitialize on the Request and Response features. https://github.com/aspnet/AspNetCore/blob/master/src/Http/Http/src/DefaultHttpContext.cs#L197.

I'm still thinking about how to write a test for this.

Resolved offline.