Skip to content

Comments

Add EnableMultipleHttp2Connections property to HttpHandlerOptions#2126

Open
EngRajabi wants to merge 8 commits intoThreeMammals:developfrom
EngRajabi:feat_enableHttp2
Open

Add EnableMultipleHttp2Connections property to HttpHandlerOptions#2126
EngRajabi wants to merge 8 commits intoThreeMammals:developfrom
EngRajabi:feat_enableHttp2

Conversation

@EngRajabi
Copy link

@EngRajabi EngRajabi commented Jul 21, 2024 •
edited by raman-m
Loading

If you activate HTTP/2, it is better to activate the SocketsHttpHandler.EnableMultipleHttp2Connections configuration as well, which gives good results.

Docs

@raman-m raman-m self-assigned this Jul 22, 2024
@raman-m
Copy link
Member

raman-m commented Jul 22, 2024 •
edited
Loading

Thank you, Mohsen!
Is prioritization required for this PR?
It seems straightforward, and we could aim to deliver it in the release following the 23.3 Hotfixes.

Currently, there is a shortage of both unit and acceptance tests to begin the review process. We need to implement a genuine HTTP/2 + The Option acceptance test to assess the stability of Ocelot Core in this scenario.

@raman-m raman-m added feature A new feature Configuration Ocelot feature: Configuration Routing Ocelot feature: Routing Oct'24 October 2024 release labels Jul 22, 2024
@raman-m raman-m added this to the Summer'24 milestone Jul 22, 2024
@raman-m raman-m changed the title feat: add EnableMultipleHttp2Connections Add EnableMultipleHttp2Connections property to HttpHandlerOptions Jul 22, 2024
@EngRajabi
Copy link
Author

EngRajabi commented Jul 23, 2024 •
edited by raman-m
Loading

It is not a high priority.
I can get a load test for it. But it is time consuming

@raman-m raman-m requested review from RaynaldM, ggnaegi and raman-m July 23, 2024 10:41
raman-m
raman-m reviewed Jul 23, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

My suggestions are below 👇

src/Ocelot/Configuration/Creator/HttpHandlerOptionsCreator.cs Outdated
Comment on lines 28 to 29
options.UseCookieContainer, useTracing, options.UseProxy, maxConnectionPerServer, pooledConnectionLifetime,
options.EnableMultipleHttp2Connections);
Copy link
Member

@raman-m raman-m Jul 23, 2024

Choose a reason for hiding this comment

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

Certainly, extending the default advanced initialization constructor with all initialization values is beneficial. However, it's also worth considering the introduction of a copy constructor.

Having the copy-constructor we can write:

            return new HttpHandlerOptions(options);

Copy link
Author

@EngRajabi EngRajabi Sep 6, 2024

Choose a reason for hiding this comment

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

@raman-m
How to transfer the conditions of lines 21, 24 and 25? Line 21 uses ITracer

src/Ocelot/Configuration/HttpHandlerOptions.cs Outdated
{
public HttpHandlerOptions(bool allowAutoRedirect, bool useCookieContainer, bool useTracing, bool useProxy,
int maxConnectionsPerServer, TimeSpan pooledConnectionLifeTime)
int maxConnectionsPerServer, TimeSpan pooledConnectionLifeTime, bool enableMultipleHttp2Connections)
Copy link
Member

@raman-m raman-m Jul 23, 2024

Choose a reason for hiding this comment

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

Lack of copy-constructor.

test/Ocelot.UnitTests/Configuration/DownstreamRouteExtensionsTests.cs Outdated
null,
null,
new HttpHandlerOptions(false, false, false, false, 0, TimeSpan.Zero),
new HttpHandlerOptions(false, false, false, false, 0, TimeSpan.Zero, false),
Copy link
Member

@raman-m raman-m Jul 23, 2024

Choose a reason for hiding this comment

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

Why not to define default constructor with default initialization values including a value for EnableMultipleHttp2Connections which is false?

test/Ocelot.UnitTests/Configuration/HttpHandlerOptionsCreatorTests.cs Outdated
.BDDfy();
}

[Fact]
Copy link
Member

@raman-m raman-m Jul 23, 2024

Choose a reason for hiding this comment

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

Lack of Traits

Copy link
Author

@EngRajabi EngRajabi Sep 6, 2024

Choose a reason for hiding this comment

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

I did not understand what you mean

Copy link
Member

@raman-m raman-m Oct 18, 2024

Choose a reason for hiding this comment

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

I mean this

Ocelot/test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs

Line 131 in ce0227c

[Trait("Issue", "1833")]

RaynaldM
RaynaldM approved these changes Aug 21, 2024
View reviewed changes
raman-m
raman-m requested changes Aug 30, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

TODO

  • Fix code review issues
  • Add unit tests
  • Add acceptance tests

@raman-m raman-m mentioned this pull request Aug 31, 2024
Open
@raman-m
Copy link
Member

raman-m commented Oct 18, 2024

@RaynaldM, why did you approve the PR? It lacks new unit tests and doesn't include any acceptance tests. Additionally, the issues I raised during the code review remain unaddressed.
Does your approval indicate that your suggestions have been implemented and you do not plan to review or enhance it further? Is that the reason for your approval?

@RaynaldM
Copy link
Contributor

RaynaldM commented Oct 24, 2024

@raman-m I don't materially have the time to explore the PRs in depth, I'm content with a surface review, more on form than substance, and so my approval is on form.

@raman-m raman-m added Summer'25 Summer 2025 release and removed Oct'24 October 2024 release labels Oct 26, 2024
@raman-m raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024
@raman-m raman-m force-pushed the develop branch 16 times, most recently from 908d84f to 0678e7a Compare April 19, 2025 15:13
@raman-m
Copy link
Member

raman-m commented May 26, 2025

Mohsen,
The merge from develop not handled correctly!
Could you please resolve the conflicts carefully to ensure a green build, free from compilation errors?

@raman-m raman-m added Winter'26 Winter 2026 release and removed Summer'25 Summer 2025 release labels May 27, 2025
@raman-m raman-m modified the milestones: Spring'25, Summer'25 May 27, 2025
@raman-m
Copy link
Member

raman-m commented Jun 30, 2025 •
edited
Loading

@EngRajabi Dear author,
Please address the compilation, as the build has failed for the last two commits. Specifically, the most recent merge from the develop branch was unsuccessful.

This is the second notification from our team: you have been issued a yellow card 🟨
If further instances of pushing bad commits (broken code) occur, you may receive a red card.

@EngRajabi
Copy link
Author

EngRajabi commented Jul 1, 2025

@EngRajabi Dear author, Please address the compilation, as the build has failed for the last two commits. Specifically, the most recent merge from the develop branch was unsuccessful.

This is the second notification from our team: you have been issued a yellow card 🟨
If further instances of pushing bad commits (broken code) occur, you may receive a red card.

Hello dear friend. You have kept this merge so long that it is outdated. I don't have time to correct the changes.

@raman-m raman-m added the conflicts Feature branch has merge conflicts label Dec 27, 2025
@raman-m raman-m removed the conflicts Feature branch has merge conflicts label Jan 18, 2026
@coveralls
Copy link
Collaborator

coveralls commented Jan 18, 2026

Coverage Status

coverage: 93.522% (+0.02%) from 93.503%
when pulling 0013c4a on EngRajabi:feat_enableHttp2
into 9fc4e78 on ThreeMammals:develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raman-m raman-m raman-m requested changes

@ggnaegi ggnaegi Awaiting requested review from ggnaegi

+2 more reviewers

@ibnuda ibnuda ibnuda left review comments

@RaynaldM RaynaldM RaynaldM approved these changes

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

@EngRajabi EngRajabi

@raman-m raman-m

Labels

Configuration Ocelot feature: Configuration feature A new feature Routing Ocelot feature: Routing Winter'26 Winter 2026 release

Projects

None yet

Milestone

Winter'26

Development

Successfully merging this pull request may close these issues.

6 participants

@EngRajabi @raman-m @RaynaldM @coveralls @ibnuda @ggnaegi