http: avoid create new socket #1242

fengmk2 · 2015-03-23T11:35:14Z

when active sockets reach max sockets limit

fengmk2 · 2015-03-23T11:36:08Z

relation joyent/node pr nodejs/node-v0.x-archive#13104

fengmk2 · 2015-03-23T11:39:04Z

This bug come from 65f6f06#diff-5f7fb0850412c6be189faeddea6c5359L87

yosuke-furukawa · 2015-03-23T14:03:18Z

hm. we did not have a test for maxSockets check.
It depends on maxSockets interpretation.
And my concern is to change existing behavior....

Fishrock123 · 2015-03-23T14:06:09Z

This interpretation of maxSockets makes sense for the conditional.

fengmk2 · 2015-03-23T14:24:47Z

The exists count >= self.maxSockets problem is when concurrency sockets is equal to maxSockets, the last socket will be destroy and a new socket will be created immediately.

fengmk2 · 2015-03-25T14:45:01Z

/cc @isaacs @TooTallNate can help me review this?

yorkie · 2015-03-25T15:02:20Z

test/parallel/test-http-agent-maxsockets.js

+var assert = require('assert');
+var http = require('http');
+var Agent = require('_http_agent').Agent;
+var EventEmitter = require('events').EventEmitter;


@fengmk2 The variable seems unnecessary because it is not used :)

bnoordhuis · 2015-03-25T16:18:28Z

How does this affect the behavior of new Agent({ maxSockets: 1 })?

fengmk2 · 2015-03-26T00:15:36Z

@bnoordhuis It will be only keep 1 socket in freeSockets list when new Agent({ maxSockets: 1, keepAlive: true }).

fengmk2 · 2015-03-26T00:21:37Z

@yorkie fixed

fengmk2 · 2015-03-26T00:24:24Z

test/parallel/test-http-agent-maxsockets.js

+  console.log('http keepalive agent maxsockets test success.');
+  agent.destroy();
+  server.close();
+}


@bnoordhuis remove process.exit now.

bnoordhuis · 2015-03-28T11:25:40Z

Can one or two other @iojs/collaborators review this as well?

fengmk2 · 2015-03-28T12:05:20Z

@bnoordhuis style fixed and console.log removed by your suggestion.

indutny · 2015-03-28T14:36:09Z

LGTM

tellnes · 2015-03-28T14:44:13Z

It is a little hard to see what is actually changed, so maybe some more description in the commit message.

Otherwise, LGTM.

fengmk2 · 2015-03-28T15:44:35Z

@tellnes how about these commit message?

http: avoid create new socket

When active sockets less than or equal to max sockets limit, 
should keep it instead of destroy it.

Example:

If maxSockets = 5 and a socket:4 become free, the freeSockets list will be:

before: socket:4 will be destroy
  freeSockets: [0], [1], [2], [3]

now: socket:4 will push to the end of list
  freeSockets: [0], [1], [2], [3], [4]

tellnes · 2015-03-28T15:47:05Z

@fengmk2 :)

LGTM

bnoordhuis · 2015-04-03T22:59:16Z

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/440/

bnoordhuis · 2015-04-04T10:22:04Z

@fengmk2 Can you rebase your PR? There are some test failures that should be fixed in HEAD.

fengmk2 · 2015-04-04T11:00:51Z

ok，I will rebase soon

Sent from my iPhone

On Apr 4, 2015, at 6:22 PM, Ben Noordhuis notifications@github.com wrote:

@fengmk2 Can you rebase your PR? There are some test failures that should be fixed in HEAD.

—
Reply to this email directly or view it on GitHub.

When active sockets less than or equal to max sockets limit, should keep it instead of destroy it. Example: If maxSockets = 5 and a socket:4 become free, the freeSockets list will be: before: socket:4 will be destroy freeSockets: [0], [1], [2], [3] now: socket:4 will push to the end of list freeSockets: [0], [1], [2], [3], [4]

fengmk2 · 2015-04-04T12:07:25Z

@bnoordhuis rebase done

fengmk2 · 2015-04-04T12:11:30Z

@bnoordhuis How to make test run again?

bnoordhuis · 2015-04-04T16:03:55Z

By asking nicely. :-) https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/442/

fengmk2 · 2015-04-14T13:39:04Z

@bnoordhuis test pass or I need to rebase again?

Fishrock123 · 2015-04-16T20:30:46Z

Ran an updated Ci here: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/524/

looking good, merging soon.

Fishrock123 · 2015-04-16T21:26:27Z

@fengmk2 is this commit log ok?

http: logically respect maxSockets

Allows the number of active sockets to equal maxSockets.
Previously it would only allow maxSockets - 1.

Edit, otherwise I can add this, but seems unnecessary:

Example:
If maxSockets = 5 and a fifth socket becomes free,
the freeSockets list will be:

before: the fifth socket will be destroyed
  freeSockets: [0], [1], [2], [3]

now: the fifth socket will be properly reusable
  freeSockets: [0], [1], [2], [3], [4]

fengmk2 · 2015-04-17T12:37:34Z

@Fishrock123 ok, thanks!

Allows the number of pooled free sockets to equal maxSockets. Previously it would only allow maxSockets - 1. PR-URL: #1242 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Christian Tellnes <christian@tellnes.no>

Fishrock123 · 2015-04-17T16:26:29Z

landed in 7956a13 - thanks.

fengmk2 · 2015-04-18T09:06:34Z

Thanks all!

mscdex added the http Issues or PRs related to the http subsystem. label Mar 23, 2015

yorkie reviewed Mar 25, 2015
View reviewed changes

fengmk2 force-pushed the fix-max-sockets-detect branch 2 times, most recently from c13bc34 to 8343963 Compare March 26, 2015 00:08

fengmk2 reviewed Mar 26, 2015
View reviewed changes

fengmk2 force-pushed the fix-max-sockets-detect branch from 8343963 to edc3162 Compare March 28, 2015 12:04

fengmk2 force-pushed the fix-max-sockets-detect branch 2 times, most recently from eb5202a to d48a31a Compare March 28, 2015 15:46

fengmk2 force-pushed the fix-max-sockets-detect branch from d48a31a to 918e270 Compare April 4, 2015 12:06

fengmk2 mentioned this pull request Apr 16, 2015

Release proposal: 1.8.0 #1436

Closed

Fishrock123 closed this Apr 17, 2015

fengmk2 deleted the fix-max-sockets-detect branch April 18, 2015 09:02

This was referenced Jul 17, 2020

[Snyk] Fix for 1 vulnerabilities pmq20/node#23

Open

[Snyk] Security upgrade ajv from 5.5.2 to 6.12.3 saeedahassan/node#20

Open

[Snyk] Security upgrade ajv from 5.5.2 to 6.12.3 olegnn/node#21

Open

[Snyk] Fix for 1 vulnerabilities erdun/node#22

Open

aliscco mentioned this pull request Apr 25, 2023

[Snyk] Fix for 19 vulnerabilities aliscco/alisco-node#172

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: avoid create new socket #1242

http: avoid create new socket #1242

fengmk2 commented Mar 23, 2015

fengmk2 commented Mar 23, 2015

fengmk2 commented Mar 23, 2015

yosuke-furukawa commented Mar 23, 2015

Fishrock123 commented Mar 23, 2015

fengmk2 commented Mar 23, 2015

fengmk2 commented Mar 25, 2015

yorkie Mar 25, 2015

bnoordhuis commented Mar 25, 2015

fengmk2 commented Mar 26, 2015

fengmk2 commented Mar 26, 2015

fengmk2 Mar 26, 2015

bnoordhuis commented Mar 28, 2015

fengmk2 commented Mar 28, 2015

indutny commented Mar 28, 2015

tellnes commented Mar 28, 2015

fengmk2 commented Mar 28, 2015

tellnes commented Mar 28, 2015

bnoordhuis commented Apr 3, 2015

bnoordhuis commented Apr 4, 2015

fengmk2 commented Apr 4, 2015

fengmk2 commented Apr 4, 2015

fengmk2 commented Apr 4, 2015

bnoordhuis commented Apr 4, 2015

fengmk2 commented Apr 14, 2015

Fishrock123 commented Apr 16, 2015

Fishrock123 commented Apr 16, 2015

fengmk2 commented Apr 17, 2015

Fishrock123 commented Apr 17, 2015

fengmk2 commented Apr 18, 2015

http: avoid create new socket #1242

http: avoid create new socket #1242

Conversation

fengmk2 commented Mar 23, 2015

fengmk2 commented Mar 23, 2015

fengmk2 commented Mar 23, 2015

yosuke-furukawa commented Mar 23, 2015

Fishrock123 commented Mar 23, 2015

fengmk2 commented Mar 23, 2015

fengmk2 commented Mar 25, 2015

yorkie Mar 25, 2015

Choose a reason for hiding this comment

bnoordhuis commented Mar 25, 2015

fengmk2 commented Mar 26, 2015

fengmk2 commented Mar 26, 2015

fengmk2 Mar 26, 2015

Choose a reason for hiding this comment

bnoordhuis commented Mar 28, 2015

fengmk2 commented Mar 28, 2015

indutny commented Mar 28, 2015

tellnes commented Mar 28, 2015

fengmk2 commented Mar 28, 2015

tellnes commented Mar 28, 2015

bnoordhuis commented Apr 3, 2015

bnoordhuis commented Apr 4, 2015

fengmk2 commented Apr 4, 2015

fengmk2 commented Apr 4, 2015

fengmk2 commented Apr 4, 2015

bnoordhuis commented Apr 4, 2015

fengmk2 commented Apr 14, 2015

Fishrock123 commented Apr 16, 2015

Fishrock123 commented Apr 16, 2015

fengmk2 commented Apr 17, 2015

Fishrock123 commented Apr 17, 2015

fengmk2 commented Apr 18, 2015