Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: upgrade write-atomic-file to support worker_threads #7680

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Jan 22, 2019

Summary

EDIT: We can probably just bump the version instead, hopefully the PR will land really quick 🙂 npm/write-file-atomic#37 (comment)

Due to worker_threads sharing process.pid with its parent, write-file-atomic doesn't really work when run inside a worker. See this example (using jest-worker@24.0.0-alpha.12):

// worker.js
const {default: Worker} = require('jest-worker');

const w = new Worker(require.resolve('./file'));

w.getStdout().pipe(process.stdout);
w.getStderr().pipe(process.stderr);

console.log('parent pid', process.pid);

Promise.all([w.hello(), w.hello(), w.hello(), w.hello(), w.hello()])
  .catch(e => {
    console.error(e);
  })
  .then(() => w.end());
// file.js
module.exports.hello = () => console.log('child pid', process.pid);
$ node -v
v10.14.1
$ node worker
parent pid 82720
child pid 82721
child pid 82721
child pid 82721
child pid 82722
child pid 82723
$ node --experimental-worker worker
parent pid 82778
child pid 82778
child pid 82778
child pid 82778
child pid 82778
child pid 82778

With this fork, we use worker_threads.threadId instead of process.pid.

To test, navigate to e2e/coverage-report and run ../../jest --coverage --coverageThreshold '{"global": {"lines": 100}}' --collectCoverageOnlyFrom cached-duplicates/a/identical.js --collectCoverageOnlyFrom cached-duplicates/b/identical.js -- identical.test.js --no-cache

Without this change, it crashes every time due to being unable to write the cache file. With these changes, it passes

Opened up PR: npm/write-file-atomic#37

Test plan

Green CI on node 11 (or at last fixed coverage tests) 🤞

@jeysal

This comment has been minimized.

@SimenB SimenB changed the title fix: fork write-atomic-file to support worker_threads fix: upgrade write-atomic-file to support worker_threads Jan 23, 2019
@SimenB
Copy link
Member Author

SimenB commented Jan 23, 2019

The fix has been merged and released upstream(:tada:), so I've changed this PR to just update the dep.

I verified that the reproduction from the OP now passes. This will still fail CI, however (see #7681)

@SimenB SimenB merged commit 0f94162 into jestjs:master Jan 23, 2019
@SimenB SimenB deleted the fork-atomic-write branch January 23, 2019 00:16
@iarna
Copy link

iarna commented Jan 23, 2019

I just released a patch release with a fix for a signal-exit leak, so you may want to bump again to 2.4.1!

@SimenB
Copy link
Member Author

SimenB commented Jan 23, 2019

Thanks for the heads-up! 0e1855e

captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants