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

Opt-in to set Connection:Close on downstream requests #1441

Open
wants to merge 5 commits into
base: release/24.0
Choose a base branch
from

Conversation

bjartekh
Copy link

Fixes / New Feature #

  • There's certain scenarios where Connection:Close is needed (i.e. disabling persistent http connections). This PR let's us be able to configure ConnectionClose: true|false under GlobalConfiguration or per Route in the Ocelot configuration files.

Proposed Changes

  • If ConnectionClose is set to true in configuration, add the Connection header with value Close to indicate to the server to not keep the connection open

Side note: When benchmarking this on Azure Kubernetes Service (AKS), we observed that we got a higher throughput when running with Connnection:Close than using the Ocelot.Provider.Kubernetes package (which queries the Kubernetes API prior to sending each request). This is one scenario where this PR is beneficial.

@bjartekh
Copy link
Author

Build failed, probably fixed by #1436 ?

stefancruz added a commit to stefancruz/Ocelot that referenced this pull request Dec 22, 2022
Conflicts:
	src/Ocelot/Requester/HttpClientBuilder.cs

Cherry picked from ThreeMammals#1441
@raman-m
Copy link
Member

raman-m commented Jun 30, 2023

Please do the following:

  • Merge PR 1
  • Rebase feature branch onto develop
  • Resolve all merge conflicts

@raman-m raman-m added the conflicts Feature branch has merge conflicts label Jul 11, 2023
@raman-m raman-m force-pushed the feature/connection-close-opt-in branch from d562ced to 05d0c97 Compare August 23, 2023 17:44
@raman-m raman-m added feature A new feature needs feedback Issue is waiting on feedback before acceptance and removed conflicts Feature branch has merge conflicts labels Aug 23, 2023
@raman-m
Copy link
Member

raman-m commented Aug 23, 2023

@bjartekh Hi Bjarte!
I thought you needed some help to resolve conflicts. 😉
The feature branch has been rebased onto ThreeMammals:develop successfully!
Welcome to code review!
Good luck!


I see that develop branch in your fork is too old.
Could you Sync fork please? So, the develop branch will be updated with all top commits!
Could you add me as collaborator to your forked repo please? I will update develop branch.


Is this PR related to an issue in backlog?

@raman-m raman-m added the 2023 Annual 2023 release label Mar 5, 2024
@raman-m raman-m added this to the Annual 2023 milestone Mar 5, 2024
@raman-m raman-m changed the base branch from develop to release/24.0 March 5, 2024 10:04
@raman-m raman-m force-pushed the feature/connection-close-opt-in branch from 05d0c97 to b0b7f66 Compare April 7, 2024 12:27
Comment on lines +123 to +134
handlers
.Select(handler => handler)
.Reverse()
.ToList()
.ForEach(handler =>
{
var delegatingHandler = handler();
delegatingHandler.InnerHandler = httpMessageHandler;
httpMessageHandler = delegatingHandler;
});
return httpMessageHandler;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
handlers
.Select(handler => handler)
.Reverse()
.ToList()
.ForEach(handler =>
{
var delegatingHandler = handler();
delegatingHandler.InnerHandler = httpMessageHandler;
httpMessageHandler = delegatingHandler;
});
return httpMessageHandler;
}
return handlers
.Reverse() // Reverse the sequence
.Aggregate(httpMessageHandler, (currentHandler, handlerFunc) =>
{
var delegatingHandler = handlerFunc();
delegatingHandler.InnerHandler = currentHandler;
return delegatingHandler;
});
return httpMessageHandler;
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeap! ToList was a bit overhead to get access to ForEach helper.
Aggregate helper is good, for sure because it is applicable for this case.
There is another coding life hack: calling All to process all elements of collection. 😃

But!... Ray, this file is redundant one! I left this file during rebasing just to keep the author's changes: see // TODO 😉
Both files are just container of new changes

  • src/Ocelot/Requester/HttpClientBuilder.cs
  • src/Ocelot/Requester/HttpClientWrapper.cs

You may check, there are no these files in develop! 🙃
This PR is not ready for reviews: it must be developed more ❗

@raman-m raman-m force-pushed the feature/connection-close-opt-in branch from b0b7f66 to 00469fc Compare April 16, 2024 16:52
@raman-m raman-m removed the request for review from binarymash April 16, 2024 16:57
@raman-m
Copy link
Member

raman-m commented Apr 16, 2024

The branch has been rebased onto release/24.0 with top commit 59b63ea !

Next steps

  • Further development is required❕
  • Changes in old design classes (HttpClientBuilder, HttpClientWrapper) must be moved to appropriate classes of the new kernel design, see commit 00469fc
  • Integration tests should be fixed ❗

@raman-m raman-m force-pushed the feature/connection-close-opt-in branch from 00469fc to c3b2416 Compare April 19, 2024 16:47
@raman-m
Copy link
Member

raman-m commented Apr 19, 2024

The build is 🟢 Wow! 🤯 😻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2023 Annual 2023 release feature A new feature needs feedback Issue is waiting on feedback before acceptance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants