#2039 Buffer request body and copy the body to downstreams during multiplexing #2050

PaulARoy · 2024-04-15T16:20:47Z

Closes #2039

Body cannot be forwarded twice on Aggregator #2039

Buffers the body of the request when multiplexing multiple routes and copy the buffered body to the downstreams.

I took consideration of all your feedbacks from the previous PR, don't hesitate to tell me if something's off, either in the code or in the process.

I looked at the benchmark tests but I'm not sure on how to integrate this correctly.

Proposed Changes

Smartly enables ASP.NET buffering for body copy
Copy the buffer to the downstream requests through a MemoryStream
Fixes broken tests due to CreateThreadContext name and parameter changes
Unit tests on CloneRequestBodyAsync (called n times when n > 1, not called for 1)
Acceptance test of body copy for multiple services

PaulARoy · 2024-04-15T16:24:00Z

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

+
+    protected virtual async Task<Stream> CopyBufferToTargetRequestAsync(HttpContext source)
+    {
+        source.Request.EnableBuffering();


This EnableBuffering() extension method comes from the AspNet framework:

HttpRequestRewindExtensions.EnableBuffering Method

This seems to be the recommended way to enable native capabilities:

Re-reading ASP.Net Core request bodies with EnableBuffering()

Feel free to discuss this solution here, I think it's pretty elegant.

We're aware of this fantastic ASP.NET feature 😄. However, as a team, we've opted not to use HttpClient and instead utilize the new HttpMessageInvoker pooling mechanism to enhance our heavy streaming capabilities.
For multiplexing, we should employ local buffering and replicate the body multiple times into downstreams.

The Q from docs

Ensure the request Body can be read multiple times. Normally buffers request bodies in memory; writes requests larger than 30K bytes to disk.

Ensure the request body can be read multiple times.

This is exactly what we need for multiplexing scenario!

🆗 After a quick reading docs I realized we need to consider more flexible version of the EnableBuffering extension 👉 EnableBuffering(HttpRequest, Int32, Int64) with params (see link) instead of most common EnableBuffering(HttpRequest) without params.
My proposal is intro new options to apply them as args for this version. That will give extra flexibility for end users to have settings for buffering in Aggregation scenarios only.

@raman-m thanks for the input, it is enabled on ASP.NET Request, not on HttpClient.

Where would you put the settings for this buffering? I thought it would be preferable to let it by default at first because this would not be enabled for requests that are not multiplexed, thus being a bit confusing.

Also buffering was not part of the story which is why I did the strict minimum buffering. Considering the remark on the previous PR on overstepping the boundaries of the bug, I think it would be best to postpone this (interesting) addition/customization to a more global buffering story.

Also buffering was not part of the story which is why I did the strict minimum buffering.

Okay, it makes sense not to deliver this feature at this time.
Let's wait for @ggnaegi ...

raman-m · 2024-04-16T09:57:01Z

Paul, thanks for re-creation the PR!

I looked at the benchmark tests but I'm not sure on how to integrate this correctly.

Benchmarks testing project is here: Ocelot.Benchmarks
So, as I said it is not required, but let @ggnaegi to decide on updating benchmarks. But he is on sick leave now...

PaulARoy · 2024-04-16T10:06:54Z

@raman-m commented on Apr 16

You're welcome! I hope this matches your expectations better.

Would you like to move forward and add benchmark tests afterwards or do you prefer to wait for @ggnaegi? I wish him well!

raman-m · 2024-04-16T10:20:28Z

Would you like to move forward and add benchmark tests afterwards or do you prefer to wait for @ggnaegi? I wish him well!

Let's wait for @ggnaegi plz!
Paul, have you tested the fix in your staging environment or locally? You don't see original HttpRequestException from Body cannot be forwarded twice on Aggregator #2039 in logs, right?

raman-m

My suggestions below 👇
I'll submit a couple of commits with tests improvements.

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

raman-m · 2024-04-16T10:44:22Z

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

+
+    protected virtual async Task<Stream> CopyBufferToTargetRequestAsync(HttpContext source)
+    {
+        source.Request.EnableBuffering();


🆗 After a quick reading docs I realized we need to consider more flexible version of the EnableBuffering extension 👉 EnableBuffering(HttpRequest, Int32, Int64) with params (see link) instead of most common EnableBuffering(HttpRequest) without params.
My proposal is intro new options to apply them as args for this version. That will give extra flexibility for end users to have settings for buffering in Aggregation scenarios only.

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

PaulARoy · 2024-04-16T13:11:20Z

Would you like to move forward and add benchmark tests afterwards or do you prefer to wait for @ggnaegi? I wish him well!

Let's wait for @ggnaegi plz!

Paul, have you tested the fix in your staging environment or locally? You don't see original HttpRequestException from Body cannot be forwarded twice on Aggregator #2039 in logs, right?

Yes, I tested locally first. Also the acceptance test can show quite efficiently the resolution (disabling the copy fails the test with the correct exception).

raman-m · 2024-04-16T13:33:25Z

Commit 8e399b0

@PaulARoy What is that?
Don't edit C# code using GitHub editor please! Never! Use Visual Studio only.
Could you remove this commit please? git reset --hard HEAD~1 should help.

PaulARoy · 2024-04-16T13:40:07Z

Commit 8e399b0

@PaulARoy What is that? Don't edit C# code using GitHub editor please! Never! Use Visual Studio only. Could you remove this commit please? git reset --hard HEAD~1 should help.

I'm fixing it yeah. I hoped it wasn't that bad – I was wrong.

…ed requests

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

raman-m

I think it's necessary to introduce additional buffering options in the middleware.
Please consider my suggestions listed below. 👇

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

raman-m · 2024-04-17T12:42:40Z

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

+    protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request, CancellationToken aborted)
+    {
+        request.EnableBuffering();


Next goal

EnableBuffering(HttpRequest, Int32, Int64)

Let's add MultiplexingMiddleware options.

Given options for buffering from FileAggregateRoute class:

public struct BufferingOptions { public int BufferThreshold; public long BufferLimit; }

Then, let's apply the options:

Suggested change

protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request, CancellationToken aborted)

{

request.EnableBuffering();

protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request, CancellationToken aborted, BufferingOptions options)

{

request.EnableBuffering(options.BufferThreshold, options.BufferLimit);

Your concerns about introducing this feature have been noted. It is designed to be non-global and solely for multiplexing, as buffering is permitted only for multiplexed requests.

@ggnaegi, could you share your thoughts on this?

raman-m

LGFM

raman-m · 2024-04-20T08:20:08Z

@ggnaegi @RaynaldM Hi Team!
Could you take a look at this multiplexer improvement please?

@ggnaegi Gui, you are key reviewer for this PR!

ggnaegi

Just 1-2 suggestions but fine otherwise, thanks

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

raman-m · 2024-04-24T08:36:26Z

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

        }
+
+        return targetBuffer;


@PaulARoy
Regarding the management of the Stream object's lifetime, it appears we are relying solely on Garbage Collector to handle it.

@PaulARoy Regarding the management of the Stream object's lifetime, it appears we are relying solely on Garbage Collector to handle it.

Yes, I assume it's disposed once the request itself is disposed by aspnet

I agree with @raman-m, it might be disposed... But since it's a clone of the original HttpContext, I'm not so sure anymore. Maybe we should try to dispose the cloned http contexts afterwards?

The principle of the IDisposable pattern is straightforward: if a class generates IDisposable objects (and since MemoryStream → Stream is IDisposable), then the class should either inherit the IDisposable interface or manage the object's lifetime manually.

Calling Dispose manually is possible, but it's not the best idea.

Inheriting the IDisposable interface is unusual for middleware, but it is a viable solution.

Utilizing the RegisterForDispose(IDisposable) method (see link) is another option, although it still necessitates implementing IDisposable.

I find the third option to be an elegant design choice. While this issue is not critical because of GC management and the PR could be merged as is, it would be ideal to address it today or simply proceed with PR merge.

The issue has been resolved in the commit 897c348 ✔️

I was wrong! The RegisterForDispose(IDisposable) method doesn't require implementation of IDisposable interface in the middleware class.

raman-m

Ready for delivery❗

raman-m · 2024-04-25T11:41:12Z

TODO

Next goal is #2039 Buffer request body and copy the body to downstreams during multiplexing #2050 (comment)

@raman-m

…s during multiplexing (ThreeMammals#2050) * feat: buffer the request body during multiplexing multiple routes * style: rename clone request body method to be more explicit * Code review by @raman-m * feat: refactor clone request method, add acceptance test for form-based requests * fix: add content-length log, refactor tests from @raman-m commit * Update requestaggregation.rst * style: reverse return condition * Register `Stream` objects for disposing by downstream `HttpResponse` --------- Co-authored-by: Paul Roy <[email protected]> Co-authored-by: Raman Maksimchuk <[email protected]>

PaulARoy commented Apr 15, 2024

View reviewed changes

raman-m mentioned this pull request Apr 16, 2024

#2039 Buffer request body and copy request body to downstreams during multiplexing #2044

Closed

raman-m assigned PaulARoy Apr 16, 2024

raman-m added bug Identified as a potential bug Aggregation Ocelot feature: Request Aggregation Spring'24 Spring 2024 release labels Apr 16, 2024

raman-m added this to the March-April'24 milestone Apr 16, 2024

raman-m reviewed Apr 16, 2024

View reviewed changes

raman-m changed the title ~~#2039 Buffer request body and copy request body to downstreams during multiplexing~~ #2039 Buffer request body and copy the body to downstreams during multiplexing Apr 16, 2024

raman-m requested review from RaynaldM, raman-m and ggnaegi April 16, 2024 11:00

PaulARoy force-pushed the feat/multiplexing-body-copy branch 3 times, most recently from 5ac5eeb to d3ff90b Compare April 16, 2024 14:21

Paul Roy and others added 4 commits April 17, 2024 13:55

feat: buffer the request body during multiplexing multiple routes

e7b0230

style: rename clone request body method to be more explicit

499dd50

Code review by @raman-m

e357cbe

feat: refactor clone request method, add acceptance test for form-bas…

5ed97d0

…ed requests

raman-m force-pushed the feat/multiplexing-body-copy branch from cdb9807 to 5ed97d0 Compare April 17, 2024 10:58

PaulARoy commented Apr 17, 2024

View reviewed changes

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs Outdated Show resolved Hide resolved

raman-m requested changes Apr 17, 2024

View reviewed changes

PaulARoy force-pushed the feat/multiplexing-body-copy branch from 4665d40 to 31e72b6 Compare April 17, 2024 12:57

fix: add content-length log, refactor tests from @raman-m commit

11d7deb

PaulARoy force-pushed the feat/multiplexing-body-copy branch from da1065e to 11d7deb Compare April 17, 2024 13:43

Update requestaggregation.rst

19a97a4

raman-m approved these changes Apr 19, 2024

View reviewed changes

ggnaegi reviewed Apr 23, 2024

View reviewed changes

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs Outdated Show resolved Hide resolved

src/Ocelot/Multiplexer/MultiplexingMiddleware.cs Outdated Show resolved Hide resolved

style: reverse return condition

33d1743

raman-m reviewed Apr 24, 2024

View reviewed changes

raman-m requested a review from ggnaegi April 24, 2024 09:43

RaynaldM approved these changes Apr 24, 2024

View reviewed changes

Register Stream objects for disposing by downstream HttpResponse

897c348

raman-m approved these changes Apr 25, 2024

View reviewed changes

raman-m merged commit 233f87a into ThreeMammals:develop Apr 25, 2024
1 check passed

raman-m mentioned this pull request Apr 25, 2024

Body cannot be forwarded twice on Aggregator #2039

Closed

PaulARoy deleted the feat/multiplexing-body-copy branch April 25, 2024 12:05

raman-m mentioned this pull request May 29, 2024

In C#, when using Ocelot, the ContentLength is 0 when forwarding data to downstream services. #2077

Closed

raman-m mentioned this pull request Jun 6, 2024

Release 23.3 | Spring 2024 #2088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2039 Buffer request body and copy the body to downstreams during multiplexing #2050

#2039 Buffer request body and copy the body to downstreams during multiplexing #2050

PaulARoy commented Apr 15, 2024 •

edited

Loading

PaulARoy Apr 15, 2024 •

edited by raman-m

Loading

raman-m Apr 16, 2024

raman-m Apr 16, 2024 •

edited

Loading

raman-m Apr 16, 2024 •

edited

Loading

PaulARoy Apr 16, 2024

raman-m Apr 17, 2024

raman-m commented Apr 16, 2024

PaulARoy commented Apr 16, 2024 •

edited by raman-m

Loading

raman-m commented Apr 16, 2024 •

edited

Loading

raman-m left a comment

raman-m Apr 16, 2024 •

edited

Loading

PaulARoy commented Apr 16, 2024

raman-m commented Apr 16, 2024

PaulARoy commented Apr 16, 2024

raman-m left a comment

raman-m Apr 17, 2024

raman-m Apr 17, 2024

raman-m left a comment

raman-m commented Apr 20, 2024

ggnaegi left a comment

raman-m Apr 24, 2024 •

edited

Loading

PaulARoy Apr 24, 2024

ggnaegi Apr 24, 2024

raman-m Apr 25, 2024 •

edited

Loading

raman-m Apr 25, 2024

raman-m left a comment

raman-m commented Apr 25, 2024

#2039 Buffer request body and copy the body to downstreams during multiplexing #2050

#2039 Buffer request body and copy the body to downstreams during multiplexing #2050

Conversation

PaulARoy commented Apr 15, 2024 • edited Loading

Closes #2039

Proposed Changes

PaulARoy Apr 15, 2024 • edited by raman-m Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

The Q from docs

raman-m Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m commented Apr 16, 2024

PaulARoy commented Apr 16, 2024 • edited by raman-m Loading

raman-m commented Apr 16, 2024 • edited Loading

raman-m left a comment

Choose a reason for hiding this comment

raman-m Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

PaulARoy commented Apr 16, 2024

raman-m commented Apr 16, 2024

PaulARoy commented Apr 16, 2024

raman-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Next goal

Choose a reason for hiding this comment

raman-m left a comment

Choose a reason for hiding this comment

raman-m commented Apr 20, 2024

ggnaegi left a comment

Choose a reason for hiding this comment

raman-m Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The issue has been resolved in the commit 897c348 ✔️

raman-m left a comment

Choose a reason for hiding this comment

Ready for delivery❗

raman-m commented Apr 25, 2024

TODO

PaulARoy commented Apr 15, 2024 •

edited

Loading

PaulARoy Apr 15, 2024 •

edited by raman-m

Loading

raman-m Apr 16, 2024 •

edited

Loading

raman-m Apr 16, 2024 •

edited

Loading

PaulARoy commented Apr 16, 2024 •

edited by raman-m

Loading

raman-m commented Apr 16, 2024 •

edited

Loading

raman-m Apr 16, 2024 •

edited

Loading

raman-m Apr 24, 2024 •

edited

Loading

raman-m Apr 25, 2024 •

edited

Loading

The issue has been resolved in the commit `897c348` ✔️