Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the ability to disable buffering #509

Merged
merged 5 commits into from
Jul 18, 2024
Merged

Fix the ability to disable buffering #509

merged 5 commits into from
Jul 18, 2024

Conversation

twsouthwick
Copy link
Member

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

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.
@twsouthwick
Copy link
Member Author

@joperezr can I get a review here?

@TaoziZ03
Copy link

TaoziZ03 commented Jul 12, 2024

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
Copy link
Member Author

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

Copy link

@TaoziZ03 TaoziZ03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@TaoziZ03
Copy link

TaoziZ03 commented Jul 16, 2024

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

Copy link

@TaoziZ03 TaoziZ03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check this.
#509 (comment)

@twsouthwick
Copy link
Member Author

twsouthwick commented Jul 16, 2024

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.

@twsouthwick twsouthwick added this to the 2.0 milestone Jul 17, 2024
@@ -16,13 +16,15 @@ namespace Microsoft.AspNetCore.SystemWebAdapters.Features;
[Experimental(Constants.ExperimentalFeatures.DiagnosticId)]
public interface IHttpResponseBufferingFeature
{
void EnableBuffering(int memoryThreshold, long? bufferLimit);
void EnableBuffering(int? memoryThreshold = default, long? bufferLimit = default);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need the defaults and nullable changes here? Trying to see if we can avoid being binary breaking here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes it easier to just enable buffering when we want the default and the default value is currently in a different layer than I wanted to call it from.

This interface is intentionally marked as "Experimental" so we can break it as much as we need


ValueTask FlushAsync();

[AllowNull]
Stream Filter { get; set; }

void DisableBuffering();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this member to the interface is of course also a breaking change. Do we expect this to cause any issues?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: this interface is intentionally marked as "Experimental" so we can break it as much as we need

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the two breaking change questions I have, changes look good to me.

@twsouthwick twsouthwick merged commit ac9ab3d into main Jul 18, 2024
5 checks passed
@twsouthwick twsouthwick deleted the disable-buffering branch July 18, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to disable buffering once enabled
3 participants