Fixes connection pool bug #86
Conversation
Now it makes use of a connection pool when necessary which should boost requests speed.
|
Here's the related ticket: http://jira.touchtunes.com/browse/BMS-8 |
.nvmrc
Outdated
| @@ -0,0 +1 @@ | |||
| 8.15.0 | |||
There was a problem hiding this comment.
I have philosophical reservations around checking-in developer specific environment artifacts like this .nvmrc' file. While it is not likely to cause issues, many devs use tj/n` or even Homebrew (uck...) to manage their Node versions and this file would be vestigial. It's not like checking in a VS Code prefs file... perhaps we should add the .nvmrc to the .gitignore?
There was a problem hiding this comment.
I removed it based on your comments. With my previous employer we always committed the .nvmrc file so that the next dev would know what version they are supposed to be running so I'm just used to committing it :D
lib/utils.js
Outdated
| */ | ||
| function createAgent(isSecure, options = {}) { | ||
| return new (isSecure ? https : http).Agent(options); | ||
| } |
There was a problem hiding this comment.
Generally looks like there there are mixed tabs and spaces in the file (see lines 23 to 26 above)...
There was a problem hiding this comment.
whoops. Good find. I fixed it
|
Please make sure you guys are updating the the version in package.json file. The last PR for vnext branch did not include an update to the version. |
lib/utils.js
Outdated
| * subsequent calls to the methods so that the same agents are reused for | ||
| * all requests | ||
| */ | ||
| module.exports.getHTTPAgent = (function () { |
There was a problem hiding this comment.
This file seems to export only one thing, so why not call it getHTTPAgent.js instead of utils.js? Or even, connectionPool.js, or agentManager.js or something more scoped to its actual functionality?
There was a problem hiding this comment.
Makes sense man. I just some changes I made based on your comment
Added all the relevant code to the http-agent.js file and also updated the unit tests for this module.
There was a problem hiding this comment.
The refactoring looks great. Before I approve, I think we need to check into how this is going to scale. I'm wondering how many connections a single agent can make, how how many requests per sec it can make on a single connection. We should definitely find a way to get numbers on this before we move this fix to production.
|
@privard: that a very valid concern. I couldn't find anything online regarding reusing agents and scaling problems. One thing that we could do if we found any issues is set the max number of allowed sockets in the agent to a # higher than 32. But I think that we should heavily test in dev first |
|
Sounds good, merge at will |
Now it makes use of a connection pool when necessary which should boost
requests speed.