#356 #695 #1924 Custom `HttpMessageInvoker` pooling #1824

ggnaegi · 2023-11-30T00:27:17Z

Closes #356 #695 #1924

After some checks, it seems impossible to use HttpClientFactory in the solution.
There are several reasons for this:

First, the HttpMessageHandler can potentially be unique for each route. In the case of a "Named" HttpClient, it's possible to use ConfigurePrimaryHttpMessageHandler, but this method will only be invoked the first time HttpClientFactory.CreateClient("Named") is called. In our case, this would mean that we are configuring as many "Named" clients as there are routes. This is not quite what we are looking for.
Second point, the use of resources. It seems that HttpMessageInvoker is potentially less resource-intensive than HttpClient. Of course, HttpMessageInvoker is not returned by HttpClientFactory and worse, if we force the cast, then we could face major performance problems.
See: HttpClient vs HttpMessageInvoker buffering issue microsoft/reverse-proxy#458

I conclude that we must implement our own pool mechanism...
Thus, I propose a first draft of a solution using solutions borrowed from StackExchange and Microsoft. It will obviously have to be tested in detail, but it serves as a basis for discussion.

Proposed Changes

Using HttpMessageInvoker instead of HttpClient
Using SocketsHttpHandler, introducing a new parameter PooledConnectionLifetime in HttpHandlerOptions
Proposing an implementation of IMessageInvokerPool, the pool of message invokers, inspired by StackExchange implementation
Removing redundant classes used in Ocelot (HttpClientBuilder, HttpClientWrapper and others)

I will provide Acceptance and Unit tests later. You should @raman-m, @RaynaldM have a look at this draft. Thanks

payloadName	Mean	Max	Op/s	Allocated
1KBPayload.json	591.3 us	648.3 us	1,691.1792	39.78 KB
16KBPayload.json	642.4 us	664.7 us	1,556.7119	82.36 KB
32KBPayload.json	650.1 us	675.2 us	1,538.2294	192.68 KB
64KBPayload.json	827.9 us	841.7 us	1,207.8715	350.25 KB
128KBPayload.json	997.5 us	1,018.1 us	1,002.5522	668.68 KB
256KBPayload.json	1,424.4 us	1,461.5 us	702.0456	1303.03 KB
512KBPayload.json	2,323.8 us	2,466.2 us	430.3273	2568.97 KB
2048KBPayload.json	8,299.8 us	8,570.5 us	120.4855	10171.14 KB
8192KBPayload.json	24,469.6 us	25,469.2 us	40.867	40619.62 KB
10MBPayload.dat	29,709.8 us	32,654.6 us	33.6589	42708.54 KB
15360KBPayload.json	54,608.4 us	70,385.4 us	18.3122	47989.27 KB
30720KBPayload.json	101,482.4 us	114,551.1 us	9.8539	95953.63 KB
100MBPayload.dat	381,869.0 us	419,428.2 us	2.6187	362479 KB
1024MBPayload.dat	3,300,482.0 us	3,444,481.6 us	0.303	5193381.84 KB

Table 1: Performance with current http client

payloadName	Mean	Max	Op/s	Allocated
1KBPayload.json	579.6 us	588.2 us	1,725.2972	39.26 KB
16KBPayload.json	619.0 us	666.1 us	1,615.4381	66.49 KB
32KBPayload.json	661.8 us	670.4 us	1,511.0611	160.79 KB
64KBPayload.json	856.2 us	888.3 us	1,167.9673	286.51 KB
128KBPayload.json	886.4 us	901.6 us	1,128.1035	540.97 KB
256KBPayload.json	1,478.0 us	1,503.4 us	676.5674	1047.99 KB
512KBPayload.json	2,539.9 us	2,570.8 us	393.713	2061.79 KB
2048KBPayload.json	8,232.4 us	9,159.4 us	121.4709	8128.46 KB
8192KBPayload.json	25,601.3 us	26,334.2 us	39.0605	32306.69 KB
10MBPayload.dat	26,567.3 us	27,575.2 us	37.6403	32300.54 KB
15360KBPayload.json	29,534.4 us	31,930.2 us	33.8588	32361.59 KB
30720KBPayload.json	100,626.1 us	103,409.9 us	9.9378	64746.37 KB
100MBPayload.dat	330,750.7 us	351,597.2 us	3.0234	258272.69 KB
1024MBPayload.dat	3,441,790.4 us	3,559,791.2 us	0.2905	4124999.17 KB

Table 2: Performance with the custom message invoker pool

raman-m · 2023-11-30T11:23:53Z

@ggnaegi Wow! So great thing... 😍 You are real coding locomotive, Gui! 😉

FYI... Delivery after #1724 , so these both PRs cannot be in the same release because of Prod testing.

src/Ocelot/Configuration/Creator/HttpHandlerOptionsCreator.cs

src/Ocelot/Requester/MessageInvokerPool.cs

raman-m · 2023-12-04T16:23:39Z

Guys, Let's return to this awesome PR after Nov'23 release.. 🆗 ❔

ggnaegi · 2023-12-11T14:40:13Z

@raman-m @RaynaldM I'm writing the test cases at the minute. No worries guys! I realized we can still simplify the logic, since the Downstreamroute is an immutable, no need to use the timeout as key parameter, the object reference is good enough. We might have problems in the future if allowing dynamic configuration though (1: empty the pool, then 2: load the new configuration).

ggnaegi · 2023-12-11T18:47:23Z

@raman-m @RaynaldM I'm writing the test cases at the minute. No worries guys! I realized we can still simplify the logic, since the Downstreamroute is an immutable, no need to use the timeout as key parameter, the object reference is good enough. We might have problems in the future if allowing dynamic configuration though (1: empty the pool, then 2: load the new configuration).

By the way, why not use the Record type instead?

RaynaldM · 2023-12-12T09:14:50Z

@raman-m @RaynaldM I'm writing the test cases at the minute. No worries guys! I realized we can still simplify the logic, since the Downstreamroute is an immutable, no need to use the timeout as key parameter, the object reference is good enough. We might have problems in the future if allowing dynamic configuration though (1: empty the pool, then 2: load the new configuration).

By the way, why not use the Record type instead?

It is a good idea

raman-m · 2023-12-16T14:00:14Z

👍

ggnaegi · 2023-12-17T00:59:50Z

Hello @raman-m and @RaynaldM I'm providing some benchmarks, it seems the memory leak has been addressed

ggnaegi · 2023-12-18T14:42:31Z

@raman-m Interesting: https://learn.microsoft.com/en-us/dotnet/core/runtime-config/garbage-collector#large-object-heap-threshold

…i/Ocelot into features/http_client_pool

…ody is not copied by the api gateway.

ggnaegi · 2024-01-17T21:03:39Z

@raman-m so, I have added a memory usage acceptance test in ContentTests. I have tried it with the current develop, and we can see a memory increase of about the payload size. In this PR, it is kept within a range of +/- 10 Mb

@raman-m but we should be careful and foresee some field tests when merged to develop, since the refactoring is quite aggressive....

raman-m · 2024-01-17T21:29:34Z

@raman-m but we should be careful and foresee some field tests when merged to develop, since the refactoring is quite aggressive....

@ggnaegi Yes, it is.
Sorry, I don't understand... What are "some field tests"?

Merging was done by you a couple of hours ago. I'll just press the button, there will be no merge conflicts with develop branch too.
So, what's the rest of work?

ggnaegi · 2024-01-17T21:31:20Z

@raman-m but we should be careful and foresee some field tests when merged to develop, since the refactoring is quite aggressive....

@ggnaegi Yes, it is. Sorry, I don't understand... What are "some field tests"?

Merging from develop was done by you a couple of hours ago. I'll just press the button, there will be no merge conflicts.

@raman-m I mean, wait on feedbacks from @RaynaldM and others before moving to release and create the packages ;-)

raman-m · 2024-01-17T21:35:17Z

Ha, making packages... No! I see you are so hot to release 🤣
There is the rest of 2 other features in the milestone.
Ok, make sense to wait for Ray's feedback...

raman-m · 2024-01-18T10:07:59Z

Ready to merge! ✔️

@ggnaegi @RaynaldM
Let me know if you agree with merging.

…tialization block vs whitespace. More: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1010.md

raman-m

Excellent job, Gui! 🤩

IamMartinPeek · 2024-02-02T10:45:11Z

Hey guys, just asking. When will this be released ? Would need that for my project 👍

ggnaegi · 2024-02-02T11:18:33Z

Hey guys, just asking. When will this be released ? Would need that for my project 👍

Hello, we have validated the behavior on production environments. It's looking good. we will finalize the release process in the next few days.

raman-m · 2024-02-11T09:10:40Z

@IamMartinPeek Soon... It will be released, I hope, on Monday, Feb 12...

Release 23.0 #1961

ggnaegi changed the title ~~Custom HttpMessageInvoker pooling~~ #356 Custom HttpMessageInvoker pooling Nov 30, 2023

raman-m assigned ggnaegi Nov 30, 2023

raman-m linked an issue Nov 30, 2023 that may be closed by this pull request

Implementation with HttpClientFactory in ASP.NET Core 2.1 #356

Closed

raman-m added feature A new feature needs feedback Issue is waiting on feedback before acceptance 2023 Annual 2023 release labels Nov 30, 2023

raman-m requested review from binarymash, jps, geffzhang, RaynaldM, philproctor, anktsrkr, sharpn, abooraja, wast, raman-m, yangzhongke and thiagoloureiro November 30, 2023 11:17

RaynaldM reviewed Nov 30, 2023

View reviewed changes

src/Ocelot/Configuration/Creator/HttpHandlerOptionsCreator.cs Show resolved Hide resolved

src/Ocelot/Requester/MessageInvokerPool.cs Outdated Show resolved Hide resolved

raman-m added this to the December'23 milestone Dec 4, 2023

ggnaegi force-pushed the features/http_client_pool branch from 4d56fa2 to 8a62fba Compare December 15, 2023 11:09

raman-m added the high High priority label Dec 16, 2023

raman-m and others added 9 commits January 17, 2024 18:53

Update src/Ocelot/Requester/MessageInvokerPool.cs

449409e

Update src/Ocelot/Requester/HttpExceptionToErrorMapper.cs

e595447

Update src/Ocelot/Responder/HttpContextResponder.cs

91839a5

Update src/Ocelot/Middleware/DownstreamResponse.cs

91fa713

Use null-coalescing operator for Nullable<int> obj

52587c1

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

9825904

Merge branch 'features/http_client_pool' of https://github.com/ggnaeg…

8c4d874

…i/Ocelot into features/http_client_pool

finalizing content tests. Making sure, that the downstream response b…

a6c07ee

…ody is not copied by the api gateway.

adapting tolerances

ea5df61

raman-m changed the title ~~#356 #695 #1924 Custom HttpMessageInvoker pooling~~ #356 #695 #1924 Custom HttpMessageInvoker pooling Jan 17, 2024

raman-m added 2 commits January 18, 2024 14:06

Disable StyleCop rule SA1010 which is in conflict with collection ini…

d0d846a

…tialization block vs whitespace. More: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1010.md

Refactor Content tests

8631a72

raman-m approved these changes Jan 18, 2024

View reviewed changes

raman-m merged commit 5e7e76b into ThreeMammals:develop Jan 18, 2024
2 checks passed

This was referenced Jan 20, 2024

Can't pass through large responses #881

Closed

Chunked encoding #928

Open

Problem with POSTing big files #884

Closed

request body size #914

Closed

ggnaegi mentioned this pull request Feb 7, 2024

Error when downloading a large file (>2Gb) #1262

Closed

raman-m mentioned this pull request Feb 11, 2024

Release 23.0 #1961

Merged

raman-m mentioned this pull request Feb 13, 2024

Release 23.0 | Artifacts | +semver: breaking #1962

Merged

RaynaldM mentioned this pull request Mar 8, 2024

#1875 Use Polly v8 syntax #1914

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#356 #695 #1924 Custom `HttpMessageInvoker` pooling #1824

#356 #695 #1924 Custom `HttpMessageInvoker` pooling #1824

ggnaegi commented Nov 30, 2023 •

edited by raman-m

Loading

raman-m commented Nov 30, 2023 •

edited

Loading

raman-m commented Dec 4, 2023

ggnaegi commented Dec 11, 2023

ggnaegi commented Dec 11, 2023

RaynaldM commented Dec 12, 2023

raman-m commented Dec 16, 2023

ggnaegi commented Dec 17, 2023

ggnaegi commented Dec 18, 2023

ggnaegi commented Jan 17, 2024 •

edited

Loading

raman-m commented Jan 17, 2024 •

edited

Loading

ggnaegi commented Jan 17, 2024 •

edited

Loading

raman-m commented Jan 17, 2024 •

edited

Loading

raman-m commented Jan 18, 2024

raman-m left a comment

IamMartinPeek commented Feb 2, 2024

ggnaegi commented Feb 2, 2024

raman-m commented Feb 11, 2024

#356 #695 #1924 Custom HttpMessageInvoker pooling #1824

#356 #695 #1924 Custom HttpMessageInvoker pooling #1824

Conversation

ggnaegi commented Nov 30, 2023 • edited by raman-m Loading

Closes #356 #695 #1924

Proposed Changes

raman-m commented Nov 30, 2023 • edited Loading

raman-m commented Dec 4, 2023

ggnaegi commented Dec 11, 2023

ggnaegi commented Dec 11, 2023

RaynaldM commented Dec 12, 2023

raman-m commented Dec 16, 2023

ggnaegi commented Dec 17, 2023

ggnaegi commented Dec 18, 2023

ggnaegi commented Jan 17, 2024 • edited Loading

raman-m commented Jan 17, 2024 • edited Loading

ggnaegi commented Jan 17, 2024 • edited Loading

raman-m commented Jan 17, 2024 • edited Loading

raman-m commented Jan 18, 2024

raman-m left a comment

Choose a reason for hiding this comment

IamMartinPeek commented Feb 2, 2024

ggnaegi commented Feb 2, 2024

raman-m commented Feb 11, 2024

#356 #695 #1924 Custom `HttpMessageInvoker` pooling #1824

#356 #695 #1924 Custom `HttpMessageInvoker` pooling #1824

ggnaegi commented Nov 30, 2023 •

edited by raman-m

Loading

raman-m commented Nov 30, 2023 •

edited

Loading

ggnaegi commented Jan 17, 2024 •

edited

Loading

raman-m commented Jan 17, 2024 •

edited

Loading

ggnaegi commented Jan 17, 2024 •

edited

Loading

raman-m commented Jan 17, 2024 •

edited

Loading