https: reuse TLS sessions in Agent #2228
Conversation
|
cc @nodejs/crypto @nodejs/collaborators |
brendanashworth
added
tls
lib/https.js
Outdated
There was a problem hiding this comment.
If session is the only one being set, maybe it is worth it to just use options.session = options.session || ...?
There was a problem hiding this comment.
Yeah, but we usually try to avoid changing the objects that are passed to the function.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Haha, I'd leave it as it is. Performance impact is minimal here.
|
Can this cause a memory leak for servers that make a lot of TLS connections to different servers? |
|
@brendanashworth good catch! I'll revise it tomorrow |
|
@brendanashworth added limit ;) |
|
Any further comments @brendanashworth ? cc @bnoordhuis @shigeki ;) |
|
@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! |
lib/_tls_wrap.js
Outdated
There was a problem hiding this comment.
FWIW Buffer.compare() is still fairly slow right now and using a manual for-loop in js land is faster.
There was a problem hiding this comment.
There was a problem hiding this comment.
Isn't there an edge case if socket.getSession() returns null?
There was a problem hiding this comment.
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.
|
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. |
|
Thanks everyone! May I ask you to take one last look at this before I'll land it? |
lib/_tls_wrap.js
Outdated
There was a problem hiding this comment.
don't care if you make this change, but could use return session.equals(next); (assuming they're both buffers).
There was a problem hiding this comment.
Good point. I'll use next.equals() here.
Fix: nodejs#1499 PR-URL: nodejs#2228 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
|
@trevnorris fixed |
|
LGTM |
Fix: #1499 PR-URL: #2228 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
|
Landed in 2ca5a3d, thank you everyone! |
PR-URL: nodejs#26433 Refs: nodejs#2228 Refs: nodejs#4252 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#26433 Refs: nodejs#2228 Refs: nodejs#4252 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #26433 Refs: #2228 Refs: #4252 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
6 participants
Fix: #1499