Let the user choose worker implementation #64

yurynix · 2023-06-18T14:20:43Z

Hi 👋
In this PR, we let the user choose worker implementation, either default thread (using worker_threads) or process (using child_process.fork()). Closes #63

Other notable changes:

I've added for most tests the workerType argument, so the tests will run for both implementations, all the Uint8Array tests are hard-coded to thread implementation, so are resource-limits tests, which is a worker_threads concept.
The test that tests infinite loop was not actually verifying the worker was able to exit, IMO, it was never getting the exit message, since it's event loop was busy with the while(true);, I've added a timer to handle that.
Since there are much more tests now, and many of those are actually spawning multiple threads/processes, IMO it's wise to limit ava's concurrency, ~~I chose the value 2 (don't have a reason why 2 🙃 )~~ concurrency 1 seems more stable on CI.

Let me know what you think 🙃

yurynix · 2023-06-18T15:42:32Z

This one is failing in a few configurations, but does not reproduce locally so far:

  auto-start › should result in spawn of the workers before the first call if active workerType: process

  e2e/auto-start.spec.js:37

   36:         // then                             
   37:         t.true(workerStartTime <= callTime);
   38:     });

need to look into it...

That is because minWorkers default is 0, so pool is immieadtly ready, but test passes in thread scenario becuase the uptime is coming from parent process.

yurynix · 2023-06-19T05:41:09Z

I think I should bring the Transport from v1, I will work on it later today.

… come alive

bgalek · 2023-06-19T08:14:26Z

This one is failing in a few configurations, but does not reproduce locally so far:

  auto-start › should result in spawn of the workers before the first call if active workerType: process

  e2e/auto-start.spec.js:37

   36:         // then                             
   37:         t.true(workerStartTime <= callTime);
   38:     });

need to look into it...

it seems that only windows is affected ;)

yurynix · 2023-06-19T08:24:42Z

That's solved @bgalek 🙃

I want to understand why the current windows tests are failing, and test it E2E on our system as well.

bgalek · 2023-06-19T08:25:06Z

nice!

We can't assume it's started immediately availible for work. Also the lazy init test used a noop which can execute quickly on 2 workers without the need to start more, depending on the impl. By using a longer task we force the pool to start more workers.

Because node's spawn() implementation is broken on windows Ref: nodejs/node#14046 https://github.com/mcollina/autocannon/pull/145/files

bgalek · 2023-06-20T06:24:05Z

package.json

@@ -1,5 +1,6 @@
 {
    "name": "worker-nodes",
+    "version": "2.5.0-alpha",


remove this version before releasing new one ;) for testing npm link should use 0.0.0 version - if that was the matter ;)

I'm running that on our project's CI, so I'm installing it via yarn like so:

yarn add worker-nodes@yurynix/node-worker-nodes#8010dc0fbc322c2022a057f758fa9207363ebceb

and then pushing to github for the CI to run.

yarn add from github fails if there is no version field unfortunately, due npm pack failing behind the scenes.

ok, I'll think how to handle this

bgalek · 2023-06-20T06:25:24Z

ready for review? :)

yurynix · 2023-06-20T07:03:12Z

@bgalek soon 🙏 I'll comment here, ok?

yurynix · 2023-06-20T10:46:23Z

lib/worker/process.js

+
+const EXIT_WAIT_TIME_MS = 200;
+
+const execArgsProcessor = {


This comes from v1, haven't wrote that myself.

yurynix · 2023-06-20T10:47:13Z

lib/worker/process.js

+    }
+};
+
+function createChannel(workerId, onConnect) {


That's adaptation from:
https://github.com/mcollina/autocannon/pull/145/files
because of
nodejs/node#14046

To make Windows work.

yurynix · 2023-06-20T10:47:52Z

lib/worker/transport.js

+const BUFFER_INITIAL_SIZE = 1024 * 1024; // 1MB
+const BUFFER_GROW_RATIO = 1.5;
+
+class Transport extends EventEmitter {


That's from v1 as well

Identify workers by their apropriate identifier

yurynix · 2023-06-20T11:34:50Z

lib/pool.js

@@ -252,28 +252,15 @@ class WorkerNodes extends EventEmitter {
     * @returns {Worker}
     */
    pickWorker() {
-        if (this.options.lazyStart) {


As far as I can tell, lazyStart only matters for startup,
i.e. do we start up to minWorkers or maxWorkers, so IMO this block can be simplified.

Also previous code would break if the worker took time to startup, that is because the called of pickWorker() in the lazyStart option would receive a worker that was not yet isProcessAlive (line 259 & 263) and would try to dispatch work to it in processQueue:

const worker = this.pickWorker(); if (worker) { this.dispatchTaskTo(worker); }

Ok, that wasn't entirely true, but it still can be simplified to:

if (this.canStartWorker() && (!this.options.lazyStart || !worker)) { this.startWorker(); }

yurynix · 2023-06-20T13:01:00Z

@bgalek now I think it's ready for review 🙃

I've also ran E2Es with it on our project (workerType: "process"), seems fine.
We are currently on worker-nodes@1.6.1

yurynix · 2023-06-22T08:02:40Z

Hey @bgalek 👋
Hope you're doing well 🙏
Just heads up, I will be AFK next week from 26/June and I'll be back on the 2nd of July.

bgalek · 2023-06-22T08:44:49Z

@yurynix LGTM, do You want to release it today so you can update it before your vacation? :)

yurynix · 2023-06-22T08:47:31Z

Thank you for taking the time to review that 🙏
No problem if you want to release it today, but unless I want my colleagues to hate me forever,
I'll upgrade to the new version once I'm back 🙃

yurynix added 3 commits June 18, 2023 17:09

Let the user choose worker implementation

c94cf75

Limit concurrency even further to see if it pleases CI

0ae9e68

Add a version field to test it in some projects

61f76a5

yurynix added 6 commits June 18, 2023 20:43

This test was passing only because threads get parent process' uptime

7ed2373

That is because minWorkers default is 0, so pool is immieadtly ready, but test passes in thread scenario becuase the uptime is coming from parent process.

Add the new option to types

208b183

Please the linter

19d5f26

Refine the type

a177b9b

Remove version, as it was.

2a3ebf6

Refine the test to better assert expectations

1c28b1a

yurynix added 6 commits June 19, 2023 09:34

Test raw data transfer on process as well

5ee7a31

Bring Transport back from v1

dfaacdc

Install required dependency

2d37dfd

Put a version so I can install it in a different project

cb058fe

Since maxWorkers & minWorkers set to 2, we should expect 2 workers to…

fd8f9c2

… come alive

Produce less noise when exiting

eecff9a

yurynix added 10 commits June 19, 2023 18:07

Bring back forgotten tests

66c2b50

Refactor data transport to use named pipes directly

579a00d

Because node's spawn() implementation is broken on windows Ref: nodejs/node#14046 https://github.com/mcollina/autocannon/pull/145/files

Try differen test structure

e6aeb1a

Kickoff pool termination on process exit

289117c

Tell AVA to use processes instead of worker_threads to run tests

4307da4

Try to see why CI fails on this one

2998f1c

Lets ensure the order

89fb4c1

Print more data on the test

e14280f

Wait explictly for workers to startup

ef3f75e

Remove no longer needed printout

8010dc0

bgalek reviewed Jun 20, 2023

View reviewed changes

Run prettier on the files to fix identation

615751c

yurynix commented Jun 20, 2023

View reviewed changes

yurynix added 4 commits June 20, 2023 14:29

Add threadId getter

b130070

Identifiy worker by it's apropriate identifier

4d24f1a

Split the workers start time more

408f509

Identify workers by their apropriate identifier

This can be simplified

9dc2115

yurynix commented Jun 20, 2023

View reviewed changes

yurynix added 4 commits June 20, 2023 14:52

Improve test

782da08

Allow worker to become idle

49ffad8

Bring back the code that was breaking the test

74f93a9

Remove printout

3feaa2c

Remove version field to align with main branch

cdaa597

bgalek approved these changes Jun 22, 2023

View reviewed changes

bgalek merged commit adb07c1 into allegro:main Jun 22, 2023

yurynix deleted the choose-worker-implementation branch June 22, 2023 08:50

Galcarmi mentioned this pull request Apr 17, 2024

Fix memory leak in WorkerProcess #69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let the user choose worker implementation #64

Let the user choose worker implementation #64

yurynix commented Jun 18, 2023 •

edited

Loading

yurynix commented Jun 18, 2023

yurynix commented Jun 19, 2023 •

edited

Loading

bgalek commented Jun 19, 2023

yurynix commented Jun 19, 2023

bgalek commented Jun 19, 2023

bgalek Jun 20, 2023 •

edited

Loading

yurynix Jun 20, 2023

bgalek Jun 20, 2023

bgalek commented Jun 20, 2023

yurynix commented Jun 20, 2023

yurynix Jun 20, 2023

yurynix Jun 20, 2023

yurynix Jun 20, 2023

yurynix Jun 20, 2023

yurynix Jun 20, 2023

yurynix commented Jun 20, 2023

yurynix commented Jun 22, 2023

bgalek commented Jun 22, 2023 •

edited

Loading

yurynix commented Jun 22, 2023

Let the user choose worker implementation #64

Let the user choose worker implementation #64

Conversation

yurynix commented Jun 18, 2023 • edited Loading

yurynix commented Jun 18, 2023

yurynix commented Jun 19, 2023 • edited Loading

bgalek commented Jun 19, 2023

yurynix commented Jun 19, 2023

bgalek commented Jun 19, 2023

bgalek Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

yurynix Jun 20, 2023

Choose a reason for hiding this comment

bgalek Jun 20, 2023

Choose a reason for hiding this comment

bgalek commented Jun 20, 2023

yurynix commented Jun 20, 2023

yurynix Jun 20, 2023

Choose a reason for hiding this comment

yurynix Jun 20, 2023

Choose a reason for hiding this comment

yurynix Jun 20, 2023

Choose a reason for hiding this comment

yurynix Jun 20, 2023

Choose a reason for hiding this comment

yurynix Jun 20, 2023

Choose a reason for hiding this comment

yurynix commented Jun 20, 2023

yurynix commented Jun 22, 2023

bgalek commented Jun 22, 2023 • edited Loading

yurynix commented Jun 22, 2023

yurynix commented Jun 18, 2023 •

edited

Loading

yurynix commented Jun 19, 2023 •

edited

Loading

bgalek Jun 20, 2023 •

edited

Loading

bgalek commented Jun 22, 2023 •

edited

Loading