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

feat: enable keep alive on configuration by default #2014

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

juanpicado
Copy link
Member

@juanpicado juanpicado commented Dec 2, 2020

Add by default the keep alive configuration for the newer installations.

Example: cc5d786

If this goes well, I'd enabled it directly for older installations which upgrade without have to change configuration in the near future if no troubles are reported.

https://github.com/request/request#requestoptions-callback
https://nodejs.org/api/http.html#http_new_agent_options

From docs

keepAlive: true,      // Keep sockets around even when there are no outstanding requests, so they can be used for future requests without having to reestablish a TCP connection. Defaults to false
maxSockets: 40, // Maximum number of sockets to allow per host. Defaults to Infinity.
maxFreeSockets: 10   // Maximum number of sockets to leave open in a free state. Only relevant if keepAlive is set to true. Defaults to 256.

@juanpicado juanpicado requested a review from a team December 2, 2020 20:27
@dnafication
Copy link
Contributor

What would be preferred approach of testing this?

@juanpicado
Copy link
Member Author

juanpicado commented Dec 3, 2020

What would be preferred approach of testing this?

Actually was tested already, there is some background behind of this, projecs as react-native-windows, create-react-app, verdaccio itself (cc5d786) or recently pnpm pnpm/registry-mock#2 pnpm/pnpm@33ef97c have need it to upgrade their settings to improve performance in CI (those free does not provide too much power). For the other side even locally a registry might not be able to open new connections either for network restrictions, firewalls ect etc, with keepAlive we fix this.

I know this will be a boost in performance, maybe should be enable by default via code if you think is better.

Copy link
Contributor

@dnafication dnafication left a comment

Choose a reason for hiding this comment

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

Sniffed around with wireshark to check the open sockets.

@juanpicado juanpicado merged commit 9171f25 into master Dec 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the feat_keep_alive branch December 4, 2020 06:37
juanpicado added a commit to verdaccio/charts that referenced this pull request Dec 24, 2020
juanpicado added a commit to verdaccio/charts that referenced this pull request Dec 27, 2020
juanpicado added a commit to verdaccio/charts that referenced this pull request Dec 28, 2020
* feat: enable keep alive by default

follow up of verdaccio/verdaccio#2014

* chore: bum up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants