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.

Sign up for GitHub

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

Jump to bottom

HttpContext copies in MultiplexingMiddleware even if 1 downstream route defined #1825

Closed
ggnaegi opened this issue Nov 30, 2023 · 1 comment · Fixed by #1826
Closed

HttpContext copies in MultiplexingMiddleware even if 1 downstream route defined #1825

ggnaegi opened this issue Nov 30, 2023 · 1 comment · Fixed by #1826
Assignees
@ggnaegi
Labels
Aggregation Ocelot feature: Request Aggregation feature A new feature Jan'24 January 2024 release merged Issue has been merged to dev and is waiting for the next release proposal Proposal for a new functionality in Ocelot
Milestone
January'24

Comments

@ggnaegi
Copy link
Member

ggnaegi commented Nov 30, 2023

Expected Behavior / New Feature

  • The Middleware code is readable and can be easily modified by contributors.
  • There is no copy of the http context when only one downstream route is defined.
  • When there is a single route, it is processed immediately, without going through the multiplexing logic of the middleware.

Actual Behavior / Motivation for New Feature

In most cases, a single downstream route is defined. In the middleware, however, a copy of the context is created, whereas in the case of a single route this is not necessary, as only one task is started and not several in parallel.

The middleware code is also very complicated to read, even though it is an important part of the application code.
@ggnaegi ggnaegi mentioned this issue Nov 30, 2023
Merged
@raman-m
Copy link
Member

raman-m commented Nov 30, 2023 •
edited
Loading

Yeah, Multiplexing feature is a hard nut...
Thank you for design review! 💪
So, I agree this feature can be refactored and redesigned a bit to improve performance...

In my opinion, this is complex feature and it affects performance, it is better to include this design rework into next upcoming release, but not Nov'23, and seems not Dec'23 releases... Probably we can work on this issue in Jan'24... 🆗 ❔

@raman-m raman-m added feature A new feature proposal Proposal for a new functionality in Ocelot labels Nov 30, 2023
@raman-m raman-m changed the title Ocelot creating http context copies in Multiplexing Middleware even if 1 downstream route defined Ocelot creates HttpContext copies in Multiplexing middleware even if 1 downstream route defined Nov 30, 2023
@raman-m raman-m linked a pull request Nov 30, 2023 that will close this issue
#1825 #1330 #1287 #1069 Improvements of the multiplexing middleware #1826
Merged
@raman-m raman-m added the Aggregation Ocelot feature: Request Aggregation label Dec 4, 2023
@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on 2023 Annual 2023 release labels Dec 4, 2023
@raman-m raman-m added this to the December'23 milestone Dec 5, 2023
@raman-m raman-m changed the title Ocelot creates HttpContext copies in Multiplexing middleware even if 1 downstream route defined Ocelot creates HttpContext copies in MultiplexingMiddleware even if 1 downstream route defined Feb 17, 2024
@raman-m raman-m changed the title Ocelot creates HttpContext copies in MultiplexingMiddleware even if 1 downstream route defined HttpContext copies in MultiplexingMiddleware even if 1 downstream route defined Feb 17, 2024
@raman-m raman-m added Jan'24 January 2024 release and removed 2023 Annual 2023 release labels Feb 22, 2024
@raman-m raman-m modified the milestones: Annual 2023, January'24 Feb 22, 2024
@raman-m raman-m removed a link to a pull request Mar 1, 2024
#1825 #1330 #1287 #1069 Improvements of the multiplexing middleware #1826
Merged
@raman-m raman-m linked a pull request Mar 1, 2024 that will close this issue
#1825 #1330 #1287 #1069 Improvements of the multiplexing middleware #1826
Merged
raman-m added a commit that referenced this issue Mar 1, 2024
@ggnaegi @raman-m
#1825 #1330 #1287 #1069 Improvements of the multiplexing middleware (#…
36986d6 
…1826)

* Cleanup of Multiplexing middleware, avoiding creating copies of the Http context if only one downstream route.

* updating comments

* preparing rebase, it's a hack.

* fixing test case "Copy_User_ToTarget",  method name has changed.

* adding some unit tests, checking that if 1 route, ProcessSingleRoute is called and no copies of context are made, if more than 2 Map is called, then more than 2 copies are created.

* Applying refactoring suggested.

* some code cleanup

* Some code cleanup in Aggregate Logic

* Cleanup in Aggregate Tests, verifying multiplexer cleanup

* Finalizing unit test, with complex keys.

* some code refactoring and applying suggestions from reviews

* Update requestaggregation.rst

Recover docs

* updating docs

* Update requestaggregation.rst

* Update requestaggregation.rst

* Code review by @raman-m

* Inherit `Steps` functionality instead of private aggregation

---------

Co-authored-by: Raman Maksimchuk <[email protected]>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Mar 1, 2024
@raman-m raman-m mentioned this issue Mar 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ggnaegi ggnaegi

Labels
Aggregation Ocelot feature: Request Aggregation feature A new feature Jan'24 January 2024 release merged Issue has been merged to dev and is waiting for the next release proposal Proposal for a new functionality in Ocelot
Projects
None yet
Milestone
January'24
Development

Successfully merging a pull request may close this issue.

#1825 #1330 #1287 #1069 Improvements of the multiplexing middleware ggnaegi/Ocelot
2 participants
@raman-m @ggnaegi