Update workerpool #203

mansona · 2021-06-06T10:55:47Z

Over the last week or so, I have been tracking down an issue that prevented some Ember apps from working/building in Node 16. After a conversation with @Turbo87 on Discord we thought that we should update the workerpool sub-dependency of ember-cli-babel@6 because that was causing the underlying issue.

I have successfully tested the change in this PR by updating the broccoli-babel-transpiler dependency to target this branch in a PR/branch in ember-cli-babel and then subsequently doing some manual(ish) yarn.lock manipulation to force an application to use that ember-cli-babel branch. You can check out the changes in this PR: ember-fastboot/ember-cli-fastboot#834 and the fact that the tests are passing should be a good indication that the experiment has worked 🎉

This PR does the following:

updates the workerpool dependency to latest (v6.1.0 had the fix we needed)
updates calls to workerpool.pool to include workerType because the default changed (and I set it to what it would have been)
fixed some assertions in tests because the messages didn't quite match (they seem to have expected just a message but were getting a full stack trace instead 🤷 )
cherry-picked updates to the CI because this PR is targeting the v6 release branch and those changes weren't made before it was cut

If we agree with my approach then we would need to cut a new v6 patch (or minor?) release after this is merged.

Let me know if you have any questions 👍

# Conflicts: # .travis.yml # appveyor.yml

rwjblue

Tests aren’t running (GH Actions won’t run when added in a fork). Let’s do the CI changes in a separate PR, all by themselves. Then we can rebase this against those changes and confirm things actually pass.

rwjblue · 2021-06-07T17:45:07Z

lib/parallel-api.js

-    pool = workerpool.pool(path.join(__dirname, 'worker.js'), { maxWorkers: JOBS });
+    pool = workerpool.pool(path.join(__dirname, 'worker.js'), {
+      maxWorkers: JOBS,
+      workerType: 'process'


Why force process here? If thread is available I don’t see why we’d want to force process.

well tests fail if we don't 😂 I don't really understand why, I was trying to do the minimum effort to get this passing

thread is the preferred approach here... fallback to process only when absolutely needed.

rwjblue · 2021-06-07T17:45:31Z

package.json

@@ -44,7 +44,7 @@
    "heimdalljs-logger": "^0.1.7",
    "json-stable-stringify": "^1.0.0",
    "rsvp": "^4.8.2",
-    "workerpool": "^2.3.0"
+    "workerpool": "^6.1.4"


This is breaking in a way that we typically do not do... 🤔

Specifically, this is changing an old major to drop support for a Node major version with the expectation of releasing without a breaking change indication. This is likely to cause exactly the kind of breakage you are trying to fix (introduced CI failures).

So I wasn't intending to drop support for any node version 🤔 the idea was that the tests should still check the correct Node version. I guess technically because workerpool aren't testing against our supported Node versions they could break older ones at any time, is that what you mean?

I guess technically because workerpool aren't testing against our supported Node versions they could break older ones at any time, is that what you mean?

Yes, exactly. They could update to (for example) async/await syntax, which would be breaking for Node 8 users (which ember-cli-babel@6 still supports).

IMO, the right fix (as I mentioned in #203 (review)) is to disable parallelizing when running on Node 16. It is the least invasive change, and has the least likely hood of breaking folks.

rwjblue · 2021-06-10T11:31:53Z

.github/workflows/ci.yml

+    strategy:
+      matrix:
+        os: ['ubuntu', 'windows', 'macOS']
+        node: ['10', '12', '14']


Doesn’t test node 16 (which is theoretically the point?)

Yes you're right I should have added that. I'll split the CI stuff and then add 16 in this PR when it's rebased 👍

rwjblue

If we think this is critical to fix, versus just saying Node 16 is the end of the line for folks that haven’t updated to ember-cli-babel@7 (which is very old at this point), I’d rather just drop support for parallelizing when running on Node 16 (and not change the workerpool dependency).

That would be done by making this return false when on Node 16:

broccoli-babel-transpiler/lib/parallel-api.js

Line 104 in fa52187

isParallelizable: isSerializable(options),

stefanpenner · 2021-06-11T17:06:20Z

updates the workerpool dependency to latest (v6.1.0 had the fix we needed)

What exactly was the fix? It may be practical to backport to an older workerpool.

mansona · 2021-06-14T12:56:28Z

So this was the change that fixed it in Node 16: https://github.com/josdejong/workerpool/pull/230/files

I had a quick look and it does seem like we could backport it because the same code does exist https://github.com/josdejong/workerpool/blob/v2.3.3/lib/WorkerHandler.js#L174 but I'm not entirely sure which part of it was failing 🤔 we couldn't use the same code change.

I was going to just try what @rwjblue was suggesting next i.e. remove the call to workerpool for any Node version > 14

stefanpenner · 2021-06-14T15:53:56Z

@mansona if you can backport, I can release

mansona · 2021-06-14T16:59:37Z

@stefanpenner it looks like the backport actually works as-is 😱 I have a branch that I've tested in Node 6 and Node 16 https://github.com/mansona/workerpool/tree/fix-node-16

Can you create a v2 release branch so that I can target it with a PR?

SergeAstapov · 2021-09-29T20:37:05Z

@mansona @stefanpenner @rwjblue workerpool@2.3.4 patch version was released. Does it mean we need to proceed with this PR?

I would assume, we only need to migrate Travis -> GH Actions.

~~workerpool was already upgraded to ^6.x in #190 already and package.json in master has "workerpool": "^6.0.2"~~
just realized this targets 6.x version

lifeart · 2021-12-23T08:34:47Z

@mansona @stefanpenner ping, any help needed here?

kevinkucharczyk · 2022-01-26T20:03:46Z

@mansona @stefanpenner @rwjblue sorry for pinging you again - is there a way to move this PR forward?

rwjblue · 2022-04-07T22:10:03Z

Closing as the fix (and a patch release) landed in the older version of workerpool

mansona and others added 6 commits June 5, 2021 19:20

update workerpool

7de6ddc

set workerType to 'process' as default was changed to thread

5ea8746

fix assertions in tests

5e533e2

Add GitHub Actions CI setup.

355051d

Updating .gitattributes content.

8234774

Remove TravisCI and AppVeyor

00ef732

# Conflicts: # .travis.yml # appveyor.yml

mansona mentioned this pull request Jun 6, 2021

Update workerpool emberjs/ember-cli-babel#409

Closed

mansona marked this pull request as ready for review June 10, 2021 09:30

rwjblue requested changes Jun 10, 2021

View reviewed changes

rwjblue reviewed Jun 10, 2021

View reviewed changes

rwjblue marked this pull request as draft June 10, 2021 15:50

rwjblue mentioned this pull request Jun 11, 2021

On Node 15, errors during build are not recoverable ember-cli/ember-cli#9404

Closed

mansona mentioned this pull request Jun 11, 2021

Update CI for v6 branch #204

Merged

This was referenced Jun 16, 2021

Can someone create a v2 release branch josdejong/workerpool#308

Closed

Update using ember-cli-update and drop support for Node 10 ember-fastboot/ember-cli-fastboot#834

Merged

rwjblue closed this Apr 7, 2022

This was referenced Nov 29, 2022

Fix CI and drop support for Node 12 mikrostew/ember-cli-fastboot#2

Closed

[WIP] Fix CI and drop support for Node 12 mikrostew/ember-cli-fastboot#3

Merged

Update CI and drop support for Node 12 ember-fastboot/ember-cli-fastboot#902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update workerpool #203

Update workerpool #203

mansona commented Jun 6, 2021 •

edited

Loading

rwjblue left a comment

rwjblue Jun 7, 2021

mansona Jun 11, 2021

stefanpenner Jun 11, 2021 •

edited

Loading

rwjblue Jun 7, 2021

mansona Jun 11, 2021

rwjblue Jun 11, 2021

rwjblue Jun 11, 2021

rwjblue Jun 10, 2021

mansona Jun 11, 2021

rwjblue left a comment •

edited

Loading

stefanpenner commented Jun 11, 2021

mansona commented Jun 14, 2021

stefanpenner commented Jun 14, 2021

mansona commented Jun 14, 2021

SergeAstapov commented Sep 29, 2021 •

edited

Loading

lifeart commented Dec 23, 2021

kevinkucharczyk commented Jan 26, 2022 •

edited

Loading

rwjblue commented Apr 7, 2022

Update workerpool #203

Update workerpool #203

Conversation

mansona commented Jun 6, 2021 • edited Loading

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner Jun 11, 2021 • edited Loading

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

rwjblue left a comment • edited Loading

Choose a reason for hiding this comment

stefanpenner commented Jun 11, 2021

mansona commented Jun 14, 2021

stefanpenner commented Jun 14, 2021

mansona commented Jun 14, 2021

SergeAstapov commented Sep 29, 2021 • edited Loading

lifeart commented Dec 23, 2021

kevinkucharczyk commented Jan 26, 2022 • edited Loading

rwjblue commented Apr 7, 2022

mansona commented Jun 6, 2021 •

edited

Loading

stefanpenner Jun 11, 2021 •

edited

Loading

rwjblue left a comment •

edited

Loading

SergeAstapov commented Sep 29, 2021 •

edited

Loading

kevinkucharczyk commented Jan 26, 2022 •

edited

Loading