refactor: Unify Client, Pool & Agent #620

ronag · 2021-03-25T11:12:21Z

Unifies Client, Pool and Agent under a single interface.
Removes Agent.get (in favor of extra args to client connect/disconnect).
Removes Client/Pool.busy in favor of return value on dispatch. (I'm not 100% sure this is possible, but I think so).
Makes connect/disconnect emit an array of entire connect/disconnect chain for introspection.
Makes 'drain' emit origin as first argument.
Makes Agent a client with all the api methods (request, stream etc...) on the prototype and same events as Client.

So Agent, Pool and Client all implement the same interface and could be used interchangeably.

Fixes:

#610
#616
#612

ronag · 2021-03-25T13:53:38Z

This needs more work but I'd like to have some initial feedback before spending more time.

ronag · 2021-03-25T13:55:24Z

@ShogunPanda

ronag · 2021-03-25T14:02:42Z

The Client interface basically becomes:

interface Client implements EventEmitter {
  dispatch(opts, handler): Boolean
  destroy(err, callback)
  'drain' (origin, clients)
  'connect' (origin, clients)
  'disconnect' (origin, clients, err)

  // These are not really necessary for the required API
  close(callback)
  closed: Boolean
  destroyed: Boolean
  connected: Integer
  size: Integer
  running: Integer
  pending: Integer,
}

mcollina · 2021-03-25T14:16:46Z

Huge +1 to this.

lib/agent.js

lib/pool.js

lib/agent.js

ShogunPanda · 2021-03-25T14:25:29Z

+1 for me as well. I'd just wait for #603 (with my improved coverage which is coming up) to land so we can rely on tests.

ronag · 2021-03-25T14:30:14Z

@jonnydgreen FYI, WIP.

jonnydgreen · 2021-03-25T14:36:20Z

Looks awesome, thanks for keeping me in the loop :)

ShogunPanda · 2021-03-30T08:49:01Z

+100 from me. I really like the new API! Go for it!

mcollina

Are benchmarks impacted by this change?

lib/agent.js

mcollina · 2021-03-30T09:12:36Z

lib/agent.js

+          .on('disconnect', this[kOnDisconnect])
+          .on('drain', this[kOnDrain])
+
+        this[kClients].set(opts.origin, new WeakRef(client))


We don’t cleanup. It’s a minor mem leak. I personally think it’s fine but would like some more opinions.

Is this what is described in #631? It should only be a temporary leak from my understanding of it.

test/agent.js

lib/agent.js

lib/dispatcher.js

test/agent.js

mcollina · 2021-03-30T09:22:49Z

@jonnydgreen do you think this API would be ok for you to support in #587

jonnydgreen · 2021-03-30T09:39:48Z

@mcollina yep, definitely! Looking at the code, I don't see any major problems when it comes to adding mocking support. If anything, I think it will make everything a lot cleaner to implement and opens up a lot of additional functionality in the mocking support as I can use setGlobalDispatcher for MockClient and MockPool as well :)

ronag · 2021-03-30T10:16:06Z

Are benchmarks impacted by this change?

We have the benchmark CI 😄 .

branch:

[1] http - no agent  x 653 ops/sec ±2.08% (78 runs sampled)
[1] http - keepalive x 746 ops/sec ±1.86% (72 runs sampled)
[1] http - keepalive - multiple sockets x 4,953 ops/sec ±2.91% (76 runs sampled)
[1] undici - pipeline x 5,311 ops/sec ±4.38% (74 runs sampled)
[1] undici - request x 6,034 ops/sec ±2.48% (75 runs sampled)
[1] undici - pool - request - multiple sockets x 6,324 ops/sec ±2.70% (78 runs sampled)
[1] undici - stream x 6,667 ops/sec ±2.34% (80 runs sampled)
[1] undici - dispatch x 6,732 ops/sec ±2.31% (78 runs sampled)
[1] undici - noop x 6,367 ops/sec ±1.61% (76 runs sampled)

master:

[1] http - no agent  x 679 ops/sec ±1.50% (79 runs sampled)
[1] http - keepalive x 727 ops/sec ±1.34% (73 runs sampled)
[1] http - keepalive - multiple sockets x 4,236 ops/sec ±3.03% (80 runs sampled)
[1] undici - pipeline x 5,056 ops/sec ±3.70% (81 runs sampled)
[1] undici - request x 5,846 ops/sec ±1.78% (80 runs sampled)
[1] undici - pool - request - multiple sockets x 5,882 ops/sec ±1.39% (82 runs sampled)
[1] undici - stream x 6,087 ops/sec ±1.69% (79 runs sampled)
[1] undici - dispatch x 6,266 ops/sec ±1.48% (81 runs sampled)
[1] undici - noop x 6,459 ops/sec ±0.91% (81 runs sampled)

Seems faster if anything.

ronag · 2021-03-30T10:18:05Z

I will run more in depth benchmarks on the same EPYC machine the README results come from before release.

Refs: #616

Ethan-Arrowood

this looks great. really happy to see the core API refactor for easier client instantiations

lib/dispatcher.js

lib/pool.js

mcollina · 2021-03-31T07:06:31Z

What would entail in solving the memory leak? Can't we do the connect/disconnect solution that we used in main?

ronag · 2021-03-31T07:18:41Z

What would entail in solving the memory leak? Can't we do the connect/disconnect solution that we used in main?

I don’t like/trust that solution but I can look into it again for node 12.

dnlup · 2021-03-31T07:27:43Z

What would entail in solving the memory leak? Can't we do the connect/disconnect solution that we used in main?

I don’t like/trust that solution but I can look into it again for node 12.

Not sure if feasible, but we could keep the FinlizationRegistry interface implemented here, and use the connect/disconnect events to trigger the kFinalizer.

mcollina · 2021-03-31T07:59:30Z

What would entail in solving the memory leak? Can't we do the connect/disconnect solution that we used in main?

I don’t like/trust that solution but I can look into it again for node 12.

Not sure if feasible, but we could keep the FinlizationRegistry interface implemented here, and use the connect/disconnect events to trigger the kFinalizer.

Only on Node v12. It's definitely better than leaking.

ronag · 2021-03-31T08:31:01Z

@mcollina @dnlup fixed, PTAL

@dnlup your idea is very elegant, I like it!

lib/agent.js

mcollina

lgtm

* refactor: Client.clients & Agent is a Client. Refs: nodejs#616 * fixup: rename clientt to dispatcher * fixup: keep promise chain * fixup * fixup * fixup

ronag added this to the 4.0 milestone Mar 25, 2021

ronag requested a review from mcollina March 25, 2021 11:12

ronag force-pushed the agent-client branch from a652c36 to c9192de Compare March 25, 2021 11:14

ronag requested a review from dnlup March 25, 2021 11:46

This comment has been minimized.

Sign in to view

ronag force-pushed the agent-client branch 5 times, most recently from 484d0fc to 4260bdf Compare March 25, 2021 13:46

ronag changed the title ~~refactor: Client.clients & Agent is a Client.~~ refactor: Unify Client, Pool & Agent Mar 25, 2021

ronag requested a review from Ethan-Arrowood March 25, 2021 13:55

dnlup reviewed Mar 25, 2021

View reviewed changes

lib/agent.js Outdated Show resolved Hide resolved

lib/pool.js Outdated Show resolved Hide resolved

lib/agent.js Show resolved Hide resolved

ronag mentioned this pull request Mar 25, 2021

Add undici mocking support #587

Merged

ronag force-pushed the agent-client branch 3 times, most recently from 491796b to 68b44e6 Compare March 27, 2021 18:18

ronag mentioned this pull request Mar 27, 2021

Minor fixes #629

Merged

ronag changed the base branch from main to minor-fixes March 27, 2021 18:25

ronag force-pushed the agent-client branch 4 times, most recently from e66d8b2 to f915b49 Compare March 27, 2021 18:31

ronag force-pushed the agent-client branch 2 times, most recently from 86d1cd9 to d88585d Compare March 30, 2021 08:19

mcollina requested changes Mar 30, 2021

View reviewed changes

ronag added 3 commits March 30, 2021 16:50

refactor: Client.clients & Agent is a Client.

2b2557c

Refs: #616

fixup: rename clientt to dispatcher

7a2eed1

fixup: keep promise chain

05c7664

ronag force-pushed the agent-client branch from fff3f7d to 05c7664 Compare March 30, 2021 14:50

Ethan-Arrowood approved these changes Mar 30, 2021

View reviewed changes

lib/dispatcher.js Show resolved Hide resolved

lib/dispatcher.js Show resolved Hide resolved

lib/pool.js Show resolved Hide resolved

fixup

8097359

ronag added 2 commits March 31, 2021 10:32

fixup

d2c7e21

fixup

963bf18

dnlup approved these changes Mar 31, 2021

View reviewed changes

dnlup reviewed Mar 31, 2021

View reviewed changes

lib/agent.js Show resolved Hide resolved

ronag requested a review from mcollina March 31, 2021 11:14

Ethan-Arrowood approved these changes Mar 31, 2021

View reviewed changes

mcollina approved these changes Mar 31, 2021

View reviewed changes

ronag merged commit 9c04cfa into main Mar 31, 2021

janpio mentioned this pull request Jun 17, 2021

fix(deps): update dependency undici to v4 prisma/prisma#7705

Closed

1 task

Uzlopak deleted the agent-client branch February 21, 2024 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Unify Client, Pool & Agent #620

refactor: Unify Client, Pool & Agent #620

ronag commented Mar 25, 2021 •

edited

This comment has been minimized.

ronag commented Mar 25, 2021

ronag commented Mar 25, 2021

ronag commented Mar 25, 2021 •

edited

mcollina commented Mar 25, 2021

ShogunPanda commented Mar 25, 2021

ronag commented Mar 25, 2021

jonnydgreen commented Mar 25, 2021

ShogunPanda commented Mar 30, 2021

mcollina left a comment

mcollina Mar 30, 2021

mcollina commented Mar 30, 2021

jonnydgreen commented Mar 30, 2021 •

edited

ronag commented Mar 30, 2021 •

edited

ronag commented Mar 30, 2021

Ethan-Arrowood left a comment

mcollina commented Mar 31, 2021

ronag commented Mar 31, 2021

dnlup commented Mar 31, 2021

mcollina commented Mar 31, 2021

ronag commented Mar 31, 2021 •

edited

mcollina left a comment

refactor: Unify Client, Pool & Agent #620

refactor: Unify Client, Pool & Agent #620

Conversation

ronag commented Mar 25, 2021 • edited

This comment has been minimized.

ronag commented Mar 25, 2021

ronag commented Mar 25, 2021

ronag commented Mar 25, 2021 • edited

mcollina commented Mar 25, 2021

ShogunPanda commented Mar 25, 2021

ronag commented Mar 25, 2021

jonnydgreen commented Mar 25, 2021

ShogunPanda commented Mar 30, 2021

mcollina left a comment

Choose a reason for hiding this comment

mcollina Mar 30, 2021

Choose a reason for hiding this comment

mcollina commented Mar 30, 2021

jonnydgreen commented Mar 30, 2021 • edited

ronag commented Mar 30, 2021 • edited

ronag commented Mar 30, 2021

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

mcollina commented Mar 31, 2021

ronag commented Mar 31, 2021

dnlup commented Mar 31, 2021

mcollina commented Mar 31, 2021

ronag commented Mar 31, 2021 • edited

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Mar 25, 2021 •

edited

ronag commented Mar 25, 2021 •

edited

jonnydgreen commented Mar 30, 2021 •

edited

ronag commented Mar 30, 2021 •

edited

ronag commented Mar 31, 2021 •

edited