Make IHttpResponseStartFeature more forgiving

[halter73]

Today, if you call ResponseBodyPipe.GetMemory() to start writing the response body without first calling IHttpResponseStartFeature.StartAsync(), GetMemory() will throw on Kestrel because the possibly-async response OnStarting callbacks haven't gotten the chance to run and we don't want to block in GetMemory() to wait for them.

Instead, we should make the behavior more forgiving by having GetMemory() return memory leased directly from the memory pool instead of the response body PipeWriter. This would continue until the first call to FlushAsync(). At that point, the OnStarting callbacks would be called, the headers would be committed, and the data written to the memory leased directly to the pool would be copied to the response body PipeWriter.

This way, calling IHttpResponseStartFeature.StartAsync() improves efficiency by avoiding copies before the first flush, but not calling StartAsync() doesn't break the app.

@davidfowl @jkotalik @Tratcher @muratg

[JamesNK]

👍

This change also has the benefit of allowing a framework like gRPC to write a buffered message to BodyPipe, and still be able to modify headers afterwards (I don't think this is a common scenario but the API is there for someone to attempt to do it)

What is the ETA for this?

[JunTaoLuo]

I believe this is going to be preview4.

[jkotalik]

I don't know what discussions have occurred already, but we debated this when we were implementing StartAsync initially. I also prototyped this change too. There were a few problems.

• Making this "Just work" will tank performance because all calls to GetMemory would require an extra copy and allocation until first write/flush.
• It removes any reason for a user to call StartAsync as it isn't required. Making this throw is clear that what someone is doing is "wrong"
• It reimplements a solid amount of pipe logic for renting/returning before first write. It wasn't pretty.

This change also has the benefit of allowing a framework like gRPC to write a buffered message to BodyPipe, and still be able to modify headers afterwards (I don't think this is a common scenario but the API is there for someone to attempt to do it)

I don't see why making this change would help that scenario. If you are replacing the PipeWriter to buffer the response, you are responsible for replacing the StartAsync feature too to make it so StartAsync doesn't call StartAsync on the original PipeWriter. See: https://github.com/aspnet/AspNetCore/blob/master/src/Middleware/ResponseCompression/src/ResponseCompressionMiddleware.cs#L57

[jkotalik]

Putting this in preview4 no matter what because either we are doing it or we aren't

[JamesNK]

I don't see why making this change would help that scenario. If you are replacing the PipeWriter to buffer the response

We're not doing that today. We just write to the BodyPipe. I thought this change would mean were could write to BodyPipe and let it decide the best place to send the data to, depending on whether StartAsync is called or not.

I'm not sure how common the scenario I mentioned would be (a user writing gRPC messages with buffering and then modifying the headers afterwards). Without this change we probably wouldn't bother with doing extra work to support it, at least in the initial gRPC release.

[jkotalik]

I thought this change would mean were could write to BodyPipe and let it decide the best place to send the data to, depending on whether StartAsync is called or not.

I see, what you would be doing would just work. Let's chat once I'm back in office.

[JamesNK]

One extra thing to add: there is currently a difference in behavior between writing to Response.BodyPipe in an actual Kestrel server vs TestServer.

Writing to BodyPipe without StartAsync in Kestrel server: exception
Writing to BodyPipe without StartAsync in TestServer: no exception

I'm guessing this is because TestServer is using StreamPipeWriter that doesn't require StartAsync to be called.

TestServer differences with the real world are concerning because we have a lot of tests are functional tests using TestServer.

[Tratcher]

Right, TestServer has an internal response pipe but it hasn't been exposed publicly and you still end up using the wrappers. Getting TestServer updated is almost as important as Kestrel, but I don't think there's an issue tracking that.

[Tratcher]

Actually, this brings up a different issue, why does StreamPipeWriter behave differently from Kestrel? Maybe it should require StartAsync. That's not TestServer's problem, it's StreamPipeWriter's.

[davidfowl]

No, we're just going to change Kestrel. It'll buffer by default and will be less efficient unless you call StartAsync. We'll do it in preview4.