Reuse tls sessions accross clients of a connection pool #588

nherment · 2021-03-05T13:58:02Z

Closes #556

This implements the reuse of TLS sessions across clients of a connection pool.

When reviewing this, please pay particular attention to:

The clearing of the TLS Sessions cache. I'm not 100% confident the logic is as good as it can be.
Whether a Map of all active sessions is that much more valuable than just keeping the last known valid session and reusing that for new clients.
The performance tests comparing session reuse vs no-reuse are reliable on my laptop but aren't on the test VMs. This is disappointing and I would appreciate if you could verify that reusing sessions is actually a net perf gain for most use cases.
There is a new option to the Pool to disable the session cache reuse tls.reuseSessions. I'm not sure if such option should be exposed but felt it is a good way to disable the feature if it causes some issues with some servers.

codecov-io · 2021-03-05T14:00:16Z

Codecov Report

Merging #588 (55803b2) into master (252b676) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #588      +/-   ##
==========================================
- Coverage   99.15%   98.88%   -0.27%     
==========================================
  Files          16       16              
  Lines        1418     1437      +19     
==========================================
+ Hits         1406     1421      +15     
- Misses         12       16       +4

Impacted Files	Coverage Δ
lib/core/symbols.js	`100.00% <ø> (ø)`
lib/core/client.js	`99.13% <100.00%> (+<0.01%)`	⬆️
lib/pool.js	`99.34% <100.00%> (+0.08%)`	⬆️
lib/node/http-parser.js	`75.00% <0.00%> (-25.00%)`	⬇️
lib/core/request.js	`94.89% <0.00%> (-2.05%)`	⬇️
lib/core/util.js	`94.73% <0.00%> (-1.32%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 252b676...55803b2. Read the comment docs.

lib/core/client.js

…te in client

lib/pool.js

…g test envs

nherment · 2021-03-05T15:11:40Z

Also please let me know if some documentation should be added to the readme

lib/pool.js

ronag

I wish we could implement this without coupling Client and Pool with hidden symbols. But i guess we can improve that in the future.

ronag · 2021-03-06T18:24:06Z

Any chance we could implement this by passing a connect method in the client options?

nherment · 2021-03-08T08:30:26Z

Any chance we could implement this by passing a connect method in the client options?

What would it help with? It'll definitely be more powerful but also come with some drawbacks if I understand correctly (e.g. re implementing the function in the pool).

ronag · 2021-03-08T09:06:05Z

What would it help with? It'll definitely be more powerful but also come with some drawbacks if I understand correctly (e.g. re implementing the function in the pool).

Removes coupling between classes.

nherment · 2021-03-08T10:53:49Z

@ronag I mainly bring 4 changes:

The Map/TLSSessionCache is gone in favor of a single session stored at the pool level. Thanks to @mcollina for this.
The Client now publicly exposes the 'session' event instead of a hidden one.
The logic to set the TLS session on top of the tls options in the pool is updated and hopefully more readable
There is a flag to enable/disable the perf test of TLS session reuse

If I understand your suggestion of passing in a connect function, I feel it would be over-complicated as the pool would need to re-implement some logic that the client owns.

I saw 2 options to address your following comment:

I wish we could implement this without coupling Client and Pool with hidden symbols.

Either implement some kind of hooks in the Client that would allow the Pool to override the default behaviour
Or publicly expose the Client session event instead of using a hidden symbol.

ronag · 2021-03-08T11:49:10Z

I think I'm happy with exposing the session event.

ronag

LGTM, good job!

nherment · 2021-03-08T12:16:59Z

@ronag or @mcollina could you please merge this for me? I don't have the access rights to do it.

ronag · 2021-03-08T12:26:58Z

Waiting for review from @mcollina

mcollina

Code is good to go!

lib/core/client.js

mcollina · 2021-03-08T16:51:56Z

test/tls-session-reuse.js

@@ -130,3 +130,103 @@ test('TLS should reuse sessions', { skip: nodeMajor < 11 }, t => {

  t.end()
 })
+
+test('A pool should be able to reuse TLS sessions between clients', { skip: nodeMajor < 11 }, t => {


What happens if this is used in node v10? Is it crashing? We should document and add a a test for it.

i don't immediately see why this wouldn't run in node 10 but i could easily be missing something obvious

@ronag @delvedor @mcollina do you remember why that was done for the Client's session reuse?
This code covers the same path so I decided to reuse the node version skip.

The session event has been added in Node v11.
Feel free to add a comment for posterity!

mcollina · 2021-03-08T16:53:54Z

Have you done any performance testing by chance? Did you see any improvement?

Ethan-Arrowood · 2021-03-09T03:31:27Z

lib/pool.js

@@ -159,11 +166,18 @@ class Pool extends EventEmitter {

      if (!client) {
        if (!this[kConnections] || this[kClients].length < this[kConnections]) {
-          client = new Client(this[kUrl], this[kOptions])
+          const options = { ...this[kOptions] }


is a call to Object.assign({}, this[kOptions]) potentially faster?

Not that I know. Cf. https://thecodebarbarian.com/object-assign-vs-object-spread.html

Ethan-Arrowood · 2021-03-09T03:41:00Z

lib/pool.js

+  }
+
+  // User has chosen to opt out of TLS session reuse
+  if (tls.reuseSessions === false) {


Suggested change

if (tls.reuseSessions === false) {

if (!tls.reuseSessions) {

Thanks for the suggestion @Ethan-Arrowood. As mentioned in the PR's description this strict equality check is on purpose.
Changing to your code will change the behavior of the pool to not reuse sessions by default.

Oh I see now

Ethan-Arrowood · 2021-03-09T03:41:15Z

lib/pool.js

            .on('drain', this[kOnDrain])
            .on('connect', this[kOnConnect])
            .on('disconnect', this[kOnDisconnect])

+          if (!options.tls || (options.tls.reuseSessions !== false && !options.tls.session)) {


Suggested change

if (!options.tls || (options.tls.reuseSessions !== false && !options.tls.session)) {

if (!options.tls || (options.tls.reuseSessions && !options.tls.session)) {

The same comment as above regarding the default behavior applies here too.

Ethan-Arrowood · 2021-03-09T03:51:51Z

test/tls-session-reuse.js

@@ -130,3 +130,103 @@ test('TLS should reuse sessions', { skip: nodeMajor < 11 }, t => {

  t.end()
 })
+
+test('A pool should be able to reuse TLS sessions between clients', { skip: nodeMajor < 11 }, t => {


i don't immediately see why this wouldn't run in node 10 but i could easily be missing something obvious

nherment · 2021-03-09T09:39:25Z

@mcollina regarding the perf tests I saw an improvement. You can test it locally. Unfortunately the perf tests dont always pass in the test VMs.

mcollina

lgtm

ronag

I'd like to understand the node 10 thing

nherment · 2021-03-09T10:20:25Z

Comment added regarding the node >= 11 version for the TLS session reuse tests.

delvedor

LGTM

Ethan-Arrowood

Great work!

nherment added 2 commits March 5, 2021 09:06

Basic implementation of TLS session reuse in a conn. pool [WIP]

dac5db6

Improved pool TLS session cache + tests

d01de4c

ronag reviewed Mar 5, 2021

View reviewed changes

lib/core/client.js Outdated Show resolved Hide resolved

nherment added 2 commits March 5, 2021 15:25

Additional tweaks to TLS session reuse in pool, making tls opts priva…

166f562

…te in client

Disabling TLS conn pool session reuse perf test b/c it's unreliable

f2c1c46

nherment commented Mar 5, 2021

View reviewed changes

lib/pool.js Outdated Show resolved Hide resolved

Reduce the # of http requests made in TLS session to avoid overloadin…

db4192e

…g test envs

nherment marked this pull request as ready for review March 5, 2021 15:10

nherment requested a review from ronag March 5, 2021 15:10

ronag reviewed Mar 5, 2021

View reviewed changes

lib/pool.js Outdated Show resolved Hide resolved

ronag reviewed Mar 5, 2021

View reviewed changes

lib/pool.js Outdated Show resolved Hide resolved

ronag reviewed Mar 5, 2021

View reviewed changes

Pool: use TLS session from options if present

ad3e8bf

nherment mentioned this pull request Mar 8, 2021

Reuse TLS sessions #556

Closed

Expose client socket session event (TLS) + simplify pool TLS sess. cache

55803b2

ronag approved these changes Mar 8, 2021

View reviewed changes

mcollina reviewed Mar 8, 2021

View reviewed changes

Ethan-Arrowood requested changes Mar 9, 2021

View reviewed changes

mcollina approved these changes Mar 9, 2021

View reviewed changes

ronag requested changes Mar 9, 2021

View reviewed changes

Add comment in test: node >= v11 for TLS session reuse

7d07bae

ronag approved these changes Mar 9, 2021

View reviewed changes

delvedor approved these changes Mar 9, 2021

View reviewed changes

Ethan-Arrowood approved these changes Mar 9, 2021

View reviewed changes

mcollina merged commit d27559e into nodejs:master Mar 10, 2021

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

Reuse tls sessions accross clients of a connection pool (nodejs#588)

3016ca7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse tls sessions accross clients of a connection pool #588

Reuse tls sessions accross clients of a connection pool #588

nherment commented Mar 5, 2021 •

edited

codecov-io commented Mar 5, 2021 •

edited

nherment commented Mar 5, 2021

ronag left a comment

ronag commented Mar 6, 2021

nherment commented Mar 8, 2021

ronag commented Mar 8, 2021 •

edited

nherment commented Mar 8, 2021

ronag commented Mar 8, 2021

ronag left a comment

nherment commented Mar 8, 2021

ronag commented Mar 8, 2021

mcollina left a comment

mcollina Mar 8, 2021

Ethan-Arrowood Mar 9, 2021

nherment Mar 9, 2021

delvedor Mar 9, 2021 •

edited

nherment Mar 9, 2021

mcollina commented Mar 8, 2021

Ethan-Arrowood Mar 9, 2021

nherment Mar 9, 2021

Ethan-Arrowood Mar 9, 2021

nherment Mar 9, 2021

Ethan-Arrowood Mar 9, 2021

Ethan-Arrowood Mar 9, 2021

nherment Mar 9, 2021

Ethan-Arrowood Mar 9, 2021

nherment commented Mar 9, 2021

mcollina left a comment

ronag left a comment

nherment commented Mar 9, 2021

delvedor left a comment

Ethan-Arrowood left a comment

	if (!options.tls \|\| (options.tls.reuseSessions !== false && !options.tls.session)) {
	if (!options.tls \|\| (options.tls.reuseSessions && !options.tls.session)) {

Reuse tls sessions accross clients of a connection pool #588

Reuse tls sessions accross clients of a connection pool #588

Conversation

nherment commented Mar 5, 2021 • edited

codecov-io commented Mar 5, 2021 • edited

Codecov Report

nherment commented Mar 5, 2021

ronag left a comment

Choose a reason for hiding this comment

ronag commented Mar 6, 2021

nherment commented Mar 8, 2021

ronag commented Mar 8, 2021 • edited

nherment commented Mar 8, 2021

ronag commented Mar 8, 2021

ronag left a comment

Choose a reason for hiding this comment

nherment commented Mar 8, 2021

ronag commented Mar 8, 2021

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvedor Mar 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nherment commented Mar 9, 2021

mcollina left a comment

Choose a reason for hiding this comment

ronag left a comment

Choose a reason for hiding this comment

nherment commented Mar 9, 2021

delvedor left a comment

Choose a reason for hiding this comment

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

nherment commented Mar 5, 2021 •

edited

codecov-io commented Mar 5, 2021 •

edited

ronag commented Mar 8, 2021 •

edited

delvedor Mar 9, 2021 •

edited