HttpResponse.OnCompleted isn't working as expected

[talalkhan]

I have tested this defect on .net core 2.2 and 3.0. It's a bug when using IIS Express but it's working as expected when switch to Kestrel. OnCompleted doesn't fire after response.
Steps to reproduce:
Run the following code in IIS express and put the breakpoint on "await Task.Delay(2000); " line.
You will see the response is sent in the browser and then the following code is executed when using the Kestrel server but when you switch to IIS Express, the response is not sent in the browser.

  [HttpGet]
        public  string Get()
        {
            try
            {
           
                // Some work 
                return "talal";
            }
            finally
            {

                Response.OnCompleted(async () =>
                {
                    // Put breakpoint here
                    await Task.Delay(2000); 
                });
            }
        }

[talalkhan]

I also confirmed this is happening on Azure App Service. I believe app service is running IIS. This is so annoying :)

[analogrelay]

We believe this is happening because the IIS Express code calls OnCompleted too early (before the response is written). That's certainly not ideal.

@talalkhan can you give more information on the actual scenario this blocks in your application? I don't disagree it's something we should look in to fixing, but I'd like to know how it impacts you so we can prioritize appropriately.

[Tratcher]

FYI here is where OnCompleted gets called:
https://github.com/aspnet/AspNetCore/blob/6f2b107b88cfbc7d4293103a74162f9075257ce9/src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs#L61

Not sure what would have to be called to unblock the response, I hope it's not waiting for the thread to exit. ProduceEnd only deals with error scenarios.

IIS doesn't implement CompleteAsync yet does it?

[talalkhan]

We believe this is happening because the IIS Express code calls OnCompleted too early (before the response is written). That's certainly not ideal.

@talalkhan can you give more information on the actual scenario this blocks in your application? I don't disagree it's something we should look in to fixing, but I'd like to know how it impacts you so we can prioritize appropriately.

The scenario is, our web service is called by a client using Http Post when the documents are ready and they want us to retrieve the documents. We have a method GetDocuments() that is triggered by their call via http post. Everything was working fine until they changed the requirement to this. They won't release the document until we send them acknowledgment when they call our Post method. So when I return acknowledgment back, the channel is closed and there is no way for my GetDocument() method to call them back. The only trigger to invoke GetDocument() at my end was their call to my service via Httppost to notify me that documents are ready.

[ghost]

<Deleted by @anurse ... This predated some of our automation and still had the "Needs: Author Feedback" label. We'll take a look and see if there's enough info here and if not we can ask another question :)>

[Tratcher]

Here's the problem. IIS does not release the request until PostCompletion is called, which is long after OnCompleted callbacks are run.
https://github.com/aspnet/AspNetCore/blob/b719a799ae27089c9a7415369c1db84a671ca5c6/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs#L614

I don't think it's just a matter of moving the callback though, ProcessRequestAsync needs to be completely re-organized so that all operations that touch the response can be completed, then PostCompletion, then OnCompleted, and then _application.DisposeContext.

[Tratcher]

Re-opening because the original fix introduced a regression (#20796) and was reverted (#21525).

The problem is that there are still things like ApplicationInsights that use IHttpContextAccessor to access the HttpContext in the Dispose event. Since we've already posted completion, IIS has cleaned up the request memory and it AVs.

It's not clear that we can fix this latency issue while still avoiding races like this. At best we could harden the IISHttpContext to throw ODEs rather than AVs if the context was accessed after PostCompletion. ApplicationInsights needs to be fixed before we can do that.

[BrennanConroy]

This is needed for gRPC