Reusing same http client instance #340
Conversation
Signed-off-by: Deepanshu Agarwal <deepanshu.agarwal1984@gmail.com>
DeepanshuA
changed the title
|
Amazing PR! 🤩 Could you fix the linking issues so the test pass? Then I can merge |
Thanks! Sure, will get them sorted. |
Signed-off-by: Deepanshu Agarwal <deepanshu.agarwal1984@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #340 +/- ##
===========================================
+ Coverage 25.63% 37.38% +11.74%
===========================================
Files 78 78
Lines 6194 6364 +170
Branches 205 265 +60
===========================================
+ Hits 1588 2379 +791
+ Misses 4602 3943 -659
- Partials 4 42 +38
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
LGTM @shubham1172 additional review possible? |
|
Can we also add gRPC in this scope? It will require the same |
| @@ -41,7 +41,7 @@ export default class ActorClient { | |||
| } | |||
There was a problem hiding this comment.
Does the gRPC client re-use the same instance internally? If not, can we cache it the same way as HTTP?
Signed-off-by: Deepanshu Agarwal <deepanshu.agarwal1984@gmail.com>
|
Changes look good to me! Thanks @DeepanshuA |
As discussed offline: It doesn't look quite straight-forward to cache grpc client. Instead, we plan to cache DaprClient and current PR would address only http client. I have created a separate issue #349 to track the discussed proposal. |
DeepanshuA
changed the title
Signed-off-by: Deepanshu Agarwal <deepanshu.agarwal1984@gmail.com>
DeepanshuA
dismissed stale reviews from shubham1172 and amulyavarote
via
93da04e
| const keepAlive = this.options.isKeepAlive; | ||
| const keepAliveMsecs = 30 * 1000; // it is applicable only when keepAlive is set to true | ||
|
|
||
| if(!HTTPClient.httpAgent) { |
There was a problem hiding this comment.
Resolve correct indentation please, seems to be 4 spaces instead of 2?
There was a problem hiding this comment.
My bad - corrected now.
Signed-off-by: Deepanshu Agarwal <deepanshu.agarwal1984@gmail.com>
Signed-off-by: Deepanshu Agarwal deepanshu.agarwal1984@gmail.com
Description
Change to reuse same HTTPClient instance for multiple invocations.
Basically, HTTPClient is acting as a Singleton here.
Tested in stand-alone mode with Actors. I tried to invoke 100000 actors.
Before: It led to multiple http client instances getting formed and this each time a new http agent being formed.
It resulted in "Temporary error when accepting new connections: accept tcp [::]:3500: accept4: too many open files"
After: 100000 actor invocations got successful, without the above issue.
Some potential discussions:
new HTTPClient, a user expects new instance rather than, same instance being returned behind-the-scenes. c) It also led to couple of errors with other variables (their formation being conditional)We can think of letting HTTPClient constructor to remain non-private but it can lead to leakage.
Issue reference
Please reference the issue this PR will close: #320
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: