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

[BUG] (Regression) 'maxsockets' default config changed to 'Infinity' in v7.7.0 #2978

Closed
wallrat opened this issue Mar 28, 2021 · 1 comment · Fixed by #2979
Closed

[BUG] (Regression) 'maxsockets' default config changed to 'Infinity' in v7.7.0 #2978

wallrat opened this issue Mar 28, 2021 · 1 comment · Fixed by #2979
Labels
Bug thing that needs fixing Priority 0 will get attention right away Release 7.x work is associated with a specific npm 7 release
Milestone
OSS - Sprint 27

Comments

@wallrat
Copy link
Contributor

wallrat commented Mar 28, 2021 •
edited
Loading

What / Why

The maxsockets configuration have changed from 50 -> 'Infitinity' during the refactoring of the config defaults in #2878

This change reverts to the behavior before this option was added, where npm tries to fetch all dependencies in parallell, potentially opening 1000+ connections, which causes all sorts of problems for users network environments (firewalls, proxies etc.).

When

The regression happend in #2878 and was released in v7.7.0

I can't find any discussion as to why this default should be changed, so I conclude that is clearly a regression.

Where

  • n/a

How

Current Behavior

as of v7.7.0 the default is 'Infinity'

define('maxsockets', {
  default: Infinity,
  type: Number,
  description: `
    The maximum number of connections to use per origin (protocol/host/port
    combination).
  `,
  flatten (key, obj, flatOptions) {
    flatOptions.maxSockets = obj[key]
  },
})

https://github.com/npm/cli/blob/latest/lib/utils/config/definitions.js#L1156

Steps to Reproduce

  • n/a

Expected Behavior

The old, pre v7.7.0 default:

 maxsockets: 50,

https://github.com/npm/cli/pull/2878/files#diff-f3564a1ffcb1f2144f5fac275184e907dd8310250b6e83becfb45b70ae75be43L130

Who

@isaacs (merged @2878)

References

  • n/a
wallrat added a commit to wallrat/cli that referenced this issue Mar 28, 2021
@wallrat
change 'maxsockets' default value back to 50
e392868 
The default value for 'maxsockets' was changed during the refactoring in npm#2878 from 50 to 'Inifinity', this PR changes it back to the previous value of 50.

Fixes npm#2978
@wallrat wallrat mentioned this issue Mar 28, 2021
Merged
@darcyclarke darcyclarke added Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release Priority 0 will get attention right away labels Mar 28, 2021
@darcyclarke darcyclarke mentioned this issue Mar 28, 2021
Closed
@darcyclarke darcyclarke added this to the OSS - Sprint 27 milestone Mar 28, 2021
@mfshao mfshao mentioned this issue Mar 29, 2021
Merged
ruyadorno pushed a commit to wallrat/cli that referenced this issue Mar 29, 2021
@wallrat @wraithgar @ruyadorno
change 'maxsockets' default value back to 15
8750b2b 
The default value for 'maxsockets' was changed during the refactoring
in npm#2878 from 50 to 'Inifinity', this PR changes it to the more
accurate value of 15, which was the default used in:
https://github.com/npm/make-fetch-happen/blob/785af652ec0c8f108a43004903afd2183af93904/agent.js#L15

Fixes npm#2978

PR-URL: npm#2979
Credit: @wallrat
Close: npm#2979
Reviewed-by: @ruyadorno

Co-authored-by: Gar <gar+gh@danger.computer>
@ruyadorno
Copy link
Collaborator

Fix is out in npm@7.7.6, thanks @wallrat 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug thing that needs fixing Priority 0 will get attention right away Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Milestone
OSS - Sprint 27
Development

Successfully merging a pull request may close this issue.

change 'maxsockets' default value to 15 wallrat/cli
3 participants
@wallrat @ruyadorno @darcyclarke