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

Implementation with HttpClientFactory in ASP.NET Core 2.1 #356

Closed
CESARDELATORRE opened this issue May 12, 2018 · 12 comments · Fixed by #1824
Closed

Implementation with HttpClientFactory in ASP.NET Core 2.1 #356

CESARDELATORRE opened this issue May 12, 2018 · 12 comments · Fixed by #1824
Assignees
@raman-m
Labels
feature A new feature merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release
Milestone
Nov-December'23

Comments

@CESARDELATORRE
Copy link

CESARDELATORRE commented May 12, 2018 •
edited by raman-m
Loading

Hey Tom! - Are you planning to move to HttpClientFactory in ASP.NET Core 2.1 when moving Ocelot to .NET Core 2.1 in the upcoming future? :)

See a summary from Glenn in my team:
https://blogs.msdn.microsoft.com/webdev/2018/02/28/asp-net-core-2-1-preview1-introducing-httpclient-factory/

Update on Nov 29, 2023
@TomPallister
Copy link
Member

@CESARDELATORRE yes, the factory looks really good and I will be changing Ocelot to use it. I will keep this issue open.

@TomPallister TomPallister added feature A new feature medium effort Likely a few days of development effort labels May 13, 2018
@TomPallister TomPallister changed the title Question - Implementation with HttpClientFactory in ASP.NET Core 2.1 New Feature - Implementation with HttpClientFactory in ASP.NET Core 2.1 May 13, 2018
@TomPallister TomPallister changed the title New Feature - Implementation with HttpClientFactory in ASP.NET Core 2.1 Implementation with HttpClientFactory in ASP.NET Core 2.1 May 14, 2018
@dbarkwell
Copy link

I was playing around with it. I got it working (some of the tests are failing though) but something tells me it can be implemented differently. @TomPallister I would like to know your thoughts: https://github.com/dbarkwell/Ocelot/tree/net-21

@TomPallister
Copy link
Member

@dbarkwell sounds good! I will take a look ASAP. I'm super busy at the moment (started a new job which is taking a lot of time)!

@binarymash
Copy link
Contributor

@TomPallister fyi AppVeyor doesn't yet support dotnet core 2.1. Their VS2017 preview image does have the RC, but another of my projects is failing with this after upgrading today because of the new Microsoft.AspNetCore.App package (which replaces Microsoft.AspNetCore.All) still looking for RC dependencies.

No big deal though cos AppVeyor say it should be up and running later this week. Gives you a bit more time to get your work done ;)

@TomPallister
Copy link
Member

@binarymash thanks for the info chief! I don’t think I’m going to be on the bleeding edge like you!!!!

@TomPallister
Copy link
Member

@dbarkwell finally had a chance to look at this, your code looks like a good start apart from changing Ocelot to target netcoreapp2.1, it supports netstandard so it can be used in classic .net projects. Unless something has changed that I have missed netcoreapp does not work in classic .net?

@stefankip
Copy link

Any updates regarding this?

@jmezach
Copy link
Contributor

jmezach commented Jun 24, 2019

Is anyone working on this? This would be a really great addition to Ocelot and could help with performance as well. I don't think it's necessary to change the target framework to netcoreapp2.1, because the Microsoft.Extensions.Http package seems to target netstandard2.0.

@geffzhang
Copy link
Contributor

Is anyone working on this? This would be a really great addition to Ocelot and could help with performance as well. I don't think it's necessary to change the target framework to netcoreapp2.1, because the Microsoft.Extensions.Http package seems to target netstandard2.0.

This does help improve performance

@jmezach
Copy link
Contributor

jmezach commented Feb 12, 2020

Wouldn't mind taking a stab at this, although I don't necessarily know where to get started. If someone can point me in the right direction that would be great. Otherwise I would have to find some time to dig into how things currently work to figure out how we can change that to incorporate the Http Client Factory.

@raman-m
Copy link
Member

raman-m commented Nov 29, 2023 •
edited
Loading

@CESARDELATORRE commented on May 12, 2018:

Hey Tom! -

I'm Tom's representative! And Tom is not at home! 😉

Thank you for pointing our attention to HttpClientFactory problem! 5 years ago! 🤯
But we still don't use IHttpClientFactory interface in Ocelot in 2023... 😮 That's terrible.
I know, not using this interface is a significant impact on performance...
Going to include to the next upcoming release...
Hope we will be able to add IHttpClientFactory interface in a simple way.

Will you be our advisor? 😉

@raman-m raman-m added the 2023 Annual 2023 release label Nov 29, 2023
@raman-m raman-m added this to the December'23 milestone Nov 29, 2023
@ggnaegi ggnaegi mentioned this issue Nov 30, 2023
Merged
@raman-m raman-m linked a pull request Nov 30, 2023 that will close this issue
#356 #695 #1924 Custom HttpMessageInvoker pooling #1824
Merged
@raman-m raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label Nov 30, 2023
@raman-m
Copy link
Member

raman-m commented Nov 30, 2023

@ggnaegi Seems we need to discuss the issue a little bit here...

@raman-m raman-m self-assigned this Nov 30, 2023
@raman-m raman-m modified the milestones: Annual 2023, Nov-December'23 Jan 17, 2024
@raman-m raman-m added Nov'23 November 2023 release and removed 2023 Annual 2023 release labels Jan 17, 2024
raman-m added a commit that referenced this issue Jan 18, 2024
@ggnaegi @raman-m
#356 #695 #1924 Custom HttpMessageInvoker pooling (#1824)
5e7e76b 
* first version

* first working version of the new http client pool

* Some cleanup, removing classes that aren't used

* some more cleanup

* forgot passing PooledConnectionLifetime

* adding todo for connection pool and request timeouts

* some code cleanup

* ongoing process refactoring tests

* a little mistake with big effects

* Several refactorings, disposing http response message to ensure that the connection is freed. Moving errors from Polly provider to Errors\QoS.

* providing some comments

* adding response body benchmark

* some minor changes in MessageInvokerPool.

* using context.RequestAborted in responder middleware (copying the response body from downstream service to the http context)

* Fix style warnings

* code review

* moving response.Content.ReadAsStreamAsync nearer to CopyToAsync with using.
Making sure, that the content stream is disposed

* HttpResponse.Content never returns null (from .net 5 onwards)

* adding more unit tests (validating, log warning if passthrough certificate, cookies are returned, message invoker timeout)

* adding a tolerance margin

* adding new acceptance test, checking memory usage. Needs to be compared with current implementation first.

* Review tests by @raman-m

* Update src/Ocelot/Configuration/HttpHandlerOptions.cs

* Update src/Ocelot/Middleware/DownstreamResponse.cs

* Update src/Ocelot/Requester/MessageInvokerPool.cs

* Update src/Ocelot/Requester/HttpExceptionToErrorMapper.cs

* Update src/Ocelot/Responder/HttpContextResponder.cs

* Update src/Ocelot/Middleware/DownstreamResponse.cs

* Use null-coalescing operator for `Nullable<int>` obj

* some modifications in the acceptance test, adding a tolerance of 1 Mb

* finalizing content tests. Making sure, that the downstream response body is not copied by the api gateway.

* adapting tolerances

* Disable StyleCop rule SA1010 which is in conflict with collection initialization block vs whitespace.
More: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1010.md

* Refactor Content tests

---------

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 Jan 18, 2024
@raman-m raman-m removed the medium effort Likely a few days of development effort label Jan 18, 2024
@raman-m raman-m mentioned this issue Feb 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@raman-m raman-m

Labels
feature A new feature merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release
Projects
None yet
Milestone
Nov-December'23
Development

Successfully merging a pull request may close this issue.

#356 #695 #1924 Custom HttpMessageInvoker pooling ggnaegi/Ocelot
8 participants
@binarymash @geffzhang @jmezach @CESARDELATORRE @TomPallister @dbarkwell @raman-m @stefankip