dotnet / systemweb-adapters

MIT License
336 stars 59 forks source link

Fix the ability to disable buffering #509

Closed twsouthwick closed 1 month ago

twsouthwick commented 2 months ago

If buffering is turned on (either because using HttpModules or the endpoint metadata), it was broken to call ctx.Features.GetRequired<IHttpResponseBodyFeature>().DisableBuffering(). This fixes that.

Fixes #508

twsouthwick commented 2 months ago

@joperezr can I get a review here?

TaoziZ03 commented 2 months ago

There is a behavior mismatch. If buffering is disabled and something is written. The framework HttpApplication will short circuit the request pipeline and jump to RequestNotification.SendReponse and raise PreSendRequestHeaders and PreSendRequestContent events. In comparison systemweb-adapter will skip all the next stages and send the response.

HttpReponse.Flush() will expliexplicitly set RequestNotification = SendReponse. The StepManager will execute it. Following is the callstack.

>   System.Web.dll!System.Web.HttpContext.CurrentNotification.set(System.Web.RequestNotification **value = SendResponse**) Line 993 C#
    System.Web.dll!System.Web.HttpRuntime.ProcessRequestNotificationPrivate(System.Web.Hosting.IIS7WorkerRequest wr, System.Web.HttpContext context) Line 1374  C#
    System.Web.dll!System.Web.Hosting.PipelineRuntime.ProcessRequestNotificationHelper(System.IntPtr rootedObjectsPointer, System.IntPtr nativeRequestContext, System.IntPtr moduleData, int flags) Line 461    C#
    System.Web.dll!System.Web.Hosting.PipelineRuntime.ProcessRequestNotification(System.IntPtr rootedObjectsPointer, System.IntPtr nativeRequestContext, System.IntPtr moduleData, int flags) Line 386  C#
    [Native to Managed Transition]  
    [Managed to Native Transition]  
    System.Web.dll!System.Web.Hosting.IIS7WorkerRequest.ExplicitFlush() Line 1729   C#
    System.Web.dll!System.Web.HttpResponse.Flush(bool finalFlush, bool async) Line 1238 C#
    System.Web.dll!System.Web.HttpWriter.WriteFromStream(byte[] data, int offset, int size) Line 341    C#
twsouthwick commented 2 months ago

@TaoziZ03 I've moved the comment about the PreSendRequest* to a new issue and a fix is in #514

TaoziZ03 commented 2 months ago

I found a bug here.

Repro

Response.HttpContext.Features.GetRequired<IHttpResponseBufferingFeature>().DisableBuffering();
...
// The response will be sent to client after this line
// Let's say current at `RequestNotification.ExecuteRequestHandler`
Reponse.Body.Write(....);

// But the rest of events will still be raised, like
// `RequestNotification.ReleaseRequestState` and so on.

Expected behavior

Please consider to raise IHttpResponseEndFeature.EndAsync() or proper method together to align framework. https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System.Web/HttpWriter.cs#L1610

twsouthwick commented 2 months ago

I've created #518 for this one, but in the future, can you create new issues for additional issues you find? This PR is focused on enabling the ability to disable buffering - by allowing that, we may find other issues, but let's get that in and deal with any follow up issues separately.