Skip to content

https: reuse TLS sessions in Agent#2228

Closed
indutny wants to merge 1 commit intonodejs:masterfrom
indutny:fix/gh-1499
Closed

https: reuse TLS sessions in Agent#2228
indutny wants to merge 1 commit intonodejs:masterfrom
indutny:fix/gh-1499

Conversation

@indutny
Copy link
Member

@indutny indutny commented Jul 23, 2015

Fix: #1499

This was referenced Jul 23, 2015
Closed
Closed
@indutny
Copy link
Member Author

indutny commented Jul 23, 2015

cc @nodejs/crypto @nodejs/collaborators

@brendanashworth brendanashworth added tls Issues and PRs related to the tls subsystem. https Issues or PRs related to the https subsystem. labels Jul 23, 2015
brendanashworth
brendanashworth reviewed Jul 23, 2015
View reviewed changes
lib/https.js Outdated
Copy link
Contributor

@brendanashworth brendanashworth Jul 23, 2015

Choose a reason for hiding this comment

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

If session is the only one being set, maybe it is worth it to just use options.session = options.session || ...?

Copy link
Member Author

@indutny indutny Jul 23, 2015

Choose a reason for hiding this comment

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

Yeah, but we usually try to avoid changing the objects that are passed to the function.

Copy link
Contributor

@brendanashworth brendanashworth Jul 23, 2015

Choose a reason for hiding this comment

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

I think we already make a copy, that's why I'm asking. :) Not trying to be a stickler - you could say that call and I go back a little bit.

Copy link
Member Author

@indutny indutny Jul 23, 2015

Choose a reason for hiding this comment

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

Haha, I'd leave it as it is. Performance impact is minimal here.

@brendanashworth
Copy link
Contributor

brendanashworth commented Jul 23, 2015

Can this cause a memory leak for servers that make a lot of TLS connections to different servers?

@indutny
Copy link
Member Author

indutny commented Jul 23, 2015

@brendanashworth good catch! I'll revise it tomorrow

@indutny
Copy link
Member Author

indutny commented Jul 23, 2015

@brendanashworth added limit ;)

@indutny
Copy link
Member Author

indutny commented Jul 23, 2015

Any further comments @brendanashworth ? cc @bnoordhuis @shigeki ;)

@brendanashworth
Copy link
Contributor

brendanashworth commented Jul 23, 2015

@indutny 'tis cool and a great improvement on before :) but I'm not comfortable reviewing, too complicated - no further comments. Can't wait to see this merged!

mscdex
mscdex reviewed Jul 24, 2015
View reviewed changes
lib/_tls_wrap.js Outdated
Copy link
Contributor

@mscdex mscdex Jul 24, 2015

Choose a reason for hiding this comment

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

FWIW Buffer.compare() is still fairly slow right now and using a manual for-loop in js land is faster.

Copy link
Member Author

@indutny indutny Jul 24, 2015

Choose a reason for hiding this comment

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

Nope: https://gist.github.com/indutny/76579b69f072e3a25155

Copy link
Contributor

@shigeki shigeki Jul 24, 2015

Choose a reason for hiding this comment

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

Isn't there an edge case if socket.getSession() returns null?

Copy link
Member Author

@indutny indutny Jul 24, 2015

Choose a reason for hiding this comment

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

Good point, @shigeki !

Copy link
Member Author

@indutny indutny Jul 24, 2015

Choose a reason for hiding this comment

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

After some consideration - I don't think that it might be the case at this point in runtime. But I will add guard just in case.

@shigeki
Copy link
Contributor

shigeki commented Jul 24, 2015

I made several tests with/without resumption against my https server and confirmed this works fine. Good job. I put small comments for fix but LGTM.
It's not an issue but I found that TLSSocket.getPeerCertificate() returns an empty object because a server does not sent Certificate to a client in resumption. It is caused by protocol spec itself but we have to care what information is lost in resumption.

@indutny
Copy link
Member Author

indutny commented Jul 24, 2015

Thanks everyone! May I ask you to take one last look at this before I'll land it?

@indutny
Copy link
Member Author

indutny commented Jul 24, 2015

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

trevnorris
trevnorris reviewed Jul 27, 2015
View reviewed changes
lib/_tls_wrap.js Outdated
Copy link
Contributor

@trevnorris trevnorris Jul 27, 2015

Choose a reason for hiding this comment

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

don't care if you make this change, but could use return session.equals(next); (assuming they're both buffers).

Copy link
Member Author

@indutny indutny Jul 27, 2015

Choose a reason for hiding this comment

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

Good point. I'll use next.equals() here.

@indutny
https: reuse TLS sessions in Agent
7286cd1 
Fix: nodejs#1499
PR-URL: nodejs#2228
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@indutny
Copy link
Member Author

indutny commented Jul 27, 2015

@trevnorris fixed

@trevnorris
Copy link
Contributor

trevnorris commented Jul 27, 2015

LGTM

indutny added a commit that referenced this pull request Jul 27, 2015
@indutny
https: reuse TLS sessions in Agent
2ca5a3d 
Fix: #1499
PR-URL: #2228
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@indutny
Copy link
Member Author

indutny commented Jul 27, 2015

Landed in 2ca5a3d, thank you everyone!

@indutny indutny closed this Jul 27, 2015
@indutny indutny deleted the fix/gh-1499 branch July 27, 2015 18:48
@cjihrig cjihrig mentioned this pull request Jul 28, 2015
Merged
@nicolas-moteau nicolas-moteau mentioned this pull request Mar 4, 2019
Closed
3 tasks
nicolas-moteau added a commit to Orange-OpenSource/node that referenced this pull request Mar 6, 2019
@nicolas-moteau
doc: add missing https Agent maxCachedSessions
e8ec731 
Refs: nodejs#2228
Refs: nodejs#4252
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 6, 2019
@nicolas-moteau @BridgeAR
doc: add missing https Agent maxCachedSessions
d50fc51 
PR-URL: nodejs#26433
Refs: nodejs#2228
Refs: nodejs#4252
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@nicolas-moteau @BridgeAR
doc: add missing https Agent maxCachedSessions
4b1c1dc 
PR-URL: nodejs#26433
Refs: nodejs#2228
Refs: nodejs#4252
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
@nicolas-moteau @BridgeAR
doc: add missing https Agent maxCachedSessions
6cc559f 
PR-URL: #26433
Refs: #2228
Refs: #4252
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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

https Issues or PRs related to the https subsystem. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@indutny @brendanashworth @shigeki @trevnorris @mscdex @thefourtheye