search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.24k stars 1.62k forks source link

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

Closed PaulARoy closed 2 months ago

PaulARoy commented 3 months ago

Closes #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

raman-m commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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

  1. Let's wait for @ggnaegi plz!
  2. Paul, have you tested the fix in your staging environment or locally? You don't see original HttpRequestException from #2039 in logs, right?
PaulARoy commented 3 months ago

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

  1. Let's wait for @ggnaegi plz!
  2. 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 commented 3 months ago

Commit https://github.com/ThreeMammals/Ocelot/pull/2050/commits/8e399b06a463a6d3d417f89361583c1edb7a43b9

@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 commented 3 months ago

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.

raman-m commented 2 months ago

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

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

raman-m commented 2 months ago

TODO