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

http: avoid create new socket #1242

Closed
wants to merge 1 commit into from
Closed

http: avoid create new socket #1242

wants to merge 1 commit into from

Conversation

fengmk2
Copy link
Contributor

@fengmk2 fengmk2 commented Mar 23, 2015

when active sockets reach max sockets limit

@fengmk2
Copy link
Contributor Author

fengmk2 commented Mar 23, 2015

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

@fengmk2
Copy link
Contributor Author

fengmk2 commented Mar 23, 2015

This bug come from 65f6f06#diff-5f7fb0850412c6be189faeddea6c5359L87

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Mar 23, 2015
@yosuke-furukawa
Copy link
Member

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

@Fishrock123
Copy link
Contributor

This interpretation of maxSockets makes sense for the conditional.

@fengmk2
Copy link
Contributor Author

fengmk2 commented Mar 23, 2015

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
Copy link
Contributor Author

fengmk2 commented Mar 25, 2015

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

yorkie
yorkie reviewed Mar 25, 2015
View reviewed changes
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@bnoordhuis
Copy link
Member

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

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

fengmk2 commented Mar 26, 2015

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

@fengmk2
Copy link
Contributor Author

fengmk2 commented Mar 26, 2015

@yorkie fixed

fengmk2
fengmk2 reviewed Mar 26, 2015
View reviewed changes
test/parallel/test-http-agent-maxsockets.js
console.log('http keepalive agent maxsockets test success.');
agent.destroy();
server.close();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis remove process.exit now.

@bnoordhuis
Copy link
Member

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

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

fengmk2 commented Mar 28, 2015

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

@indutny
Copy link
Member

indutny commented Mar 28, 2015

LGTM

@tellnes
Copy link
Contributor

tellnes commented Mar 28, 2015

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

Otherwise, LGTM.

@fengmk2
Copy link
Contributor Author

fengmk2 commented Mar 28, 2015

@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]

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

tellnes commented Mar 28, 2015

@fengmk2 :)

LGTM

@bnoordhuis
Copy link
Member

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

@bnoordhuis
Copy link
Member

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

@fengmk2
Copy link
Contributor Author

fengmk2 commented Apr 4, 2015

ok，I will rebase soon

Sent from my iPhone

On Apr 4, 2015, at 6:22 PM, Ben Noordhuis [email protected] 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.

@fengmk2
http: avoid create new socket
918e270 
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 fengmk2 force-pushed the fix-max-sockets-detect branch from d48a31a to 918e270 Compare April 4, 2015 12:06
@fengmk2
Copy link
Contributor Author

fengmk2 commented Apr 4, 2015

@bnoordhuis rebase done

@fengmk2
Copy link
Contributor Author

fengmk2 commented Apr 4, 2015

@bnoordhuis How to make test run again?

@bnoordhuis
Copy link
Member

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

@fengmk2
Copy link
Contributor Author

fengmk2 commented Apr 14, 2015

@bnoordhuis test pass or I need to rebase again?

@fengmk2 fengmk2 mentioned this pull request Apr 16, 2015
Closed
@Fishrock123
Copy link
Contributor

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

looking good, merging soon.

@Fishrock123
Copy link
Contributor

@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
Copy link
Contributor Author

fengmk2 commented Apr 17, 2015

@Fishrock123 ok, thanks!

Fishrock123 pushed a commit that referenced this pull request Apr 17, 2015
@fengmk2 @Fishrock123
http: logically respect maxSockets
7956a13 
Allows the number of pooled free sockets to equal maxSockets.
Previously it would only allow maxSockets - 1.

PR-URL: #1242
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Christian Tellnes <[email protected]>
@Fishrock123
Copy link
Contributor

landed in 7956a13 - thanks.

@fengmk2 fengmk2 deleted the fix-max-sockets-detect branch April 18, 2015 09:02
@fengmk2
Copy link
Contributor Author

fengmk2 commented Apr 18, 2015

Thanks all!

This was referenced Jul 17, 2020
Open
Open
Open
Open
@aliscco aliscco mentioned this pull request Apr 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@fengmk2 @yosuke-furukawa @Fishrock123 @bnoordhuis @indutny @tellnes @yorkie @mscdex