[Diagnostics] Consider wrapping Response.OnStarting in error handling middleware

[pranavkm]

Based on discussion here - aspnet/Mvc#7003 (comment). Mvc uses Response.OnStarting to serialize TempData in the event a ResourceFilter OnResourceExecuted did not get to it first (e.g. if an action directly wrote to the response body). In the event serializing TempData throws (for instance if the value is a type it does not support), the server will return an empty 500 and log the exception. This is because OnStarting occurs outside the middleware pipeline and any exception handling middleware has no opportunity to observe the error.

One possible solution to this is to wrap Response.OnStarting in our error handling middleware implementations and have them show the regular error page.

[pranavkm]

cc @Tratcher \ @halter73

[Tratcher]

Note you'd also have to intercept the response body first-write's to trigger the wrapped Response.OnStarting.

[halter73]

@Tratcher Are you saying you'd need to intercept the application's response body writes so they don't corrupt the diagnostic error page? Or are you saying something else?

[Tratcher]

You'd need to intercept the first write, fire your shimmed OnStarting events, catch the exception, insert your own writes for the diagnostics page, make the user's write throw, and prevent any future user writes. You'd also have to shim the send-file feature and opaque and websocket upgrade features as they all trigger OnStarting.

[halter73]

Couldn't you decorate IHttpResponseFeature.OnStarting immediately instead of waiting to intercept the first write?

We should also make the Writes no-op instead of throw. This is what happens when an OnStarting throws without any diagnostics page.

[Tratcher]

You would decorate it immediately, but you can't let Kestrel trigger it, if it does then kestrel will send the empty 500 response for you. You have to intercept the first write to trigger the event yourself so that Kestrel never sees the write or the failure so you can still write.

[halter73]

I was wrong about the no-op. Kestrel will throw an ODE from Write if OnStarting throws. If the diagnostic middleware rethrows from OnStarting, we should be able to keep the same behavior.

As for writing a response yourself, it's possible to beat Kestrel to writing the 500. E.g. the following works:

context.Response.OnStarting(async _ => {
    try
    {
        // call wrapped OnStarting callback that throws
    }
    catch
    {
        context.Response.ContentLength = 5;
        context.Response.ContentType = "text/plain";
        await context.Response.WriteAsync("TEST!");
        throw;
    }
}, null);

Disclaimer 1: The Diagnostics middleware will either have to set the content length like above or catch the exception that will likely be thrown from the applications first write in the main middleware to ensure the chunk terminator gets written.
Disclamer 2: I have no idea if this would work with Http.Sys or ANCM in-proc.

[Tratcher]

Writing to the body inside OnStarting is asking for stack overflows, deadlocks, and all sorts of undefined behavior. That's not something we can recommend to anyone, let alone use ourselves.

[halter73]

Fair enough. Just be aware this is something that the app developer could do themselves when thinking about how diagnostics is going to wrap Response.Body.Write and Response.OnStarting. It would be good if writing in OnStarting still works after this change even if we'd never recommend doing it.

[Tratcher]

No, Write in OnStarting should not be allowed in the contract (even if functionally prohibiting it is hard). This caused a lot of problems back in Katana, some of which are:

• The server isn't expecting it so it can cause re-entrency, deadlocks, stackoverflows, etc..
• If OnStarting was triggered by the app writing to the response body you have no way to prevent that write from continuing after your OnStarting callback exits. You'd end up with a corrupted response body.
• There may be more OnStarting callbacks executed after yours. They're expecting to be able to write to the response headers and will fail if those headers have been locked/flushed.

[halter73]

I don't need any convincing it's a bad idea, but writing in OnStarting doesn't currently "cause re-entrency, deadlocks, stackoverflows, etc.." I'm just saying we should be careful this change doesn't introduce re-entrency, deadlocks, stackoverflows, etc...

[mkArtakMSFT]

Closing as there have been not much community involvement here and the fix seems to be quite risky.