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

[Bug] pnpapi.setup() in main thread not affecting worker threads #2476

Open
1 task done
clemyan opened this issue Feb 13, 2021 · 6 comments
Open
1 task done

[Bug] pnpapi.setup() in main thread not affecting worker threads #2476

clemyan opened this issue Feb 13, 2021 · 6 comments
Labels
bug Something isn't working reproducible This issue can be successfully reproduced

Comments

@clemyan
Copy link
Member

clemyan commented Feb 13, 2021

  • I'd be willing to implement a fix (need guidance)

Describe the bug

Calling pnpapi.setup() in the main thread does not set up PnP insider worker threads, causing worker threads to fail when require-ing dependencies.

This affects stuff like the eslint pnpify sdk, since that just calls pnpapi.setup() and eslint plugins may spawn worker threads.

To Reproduce

Reproduction
const fs = require('fs/promises')
const mainContent = `
const { Worker, isMainThread } = require('worker_threads')
if (isMainThread) {
  require('./.pnp.cjs').setup()
  new Worker(__filename)
} else {
  require('answer/linked.js')
}
`
await Promise.all([
  fs.writeFile('./linked.js', ''),
  fs.writeFile('./main.js', mainContent)
])

await packageJsonAndInstall({
	dependencies: {
		'answer': 'link:.'
	},
})

await expect(yarn('node', 'main.js')).resolves.toBe('')

const { execSync } = require('child_process')
// Clear NODE_OPTIONS set by the sherlock invocation
expect(() => execSync('node main.js', { env: { NODE_OPTIONS: '' }, encoding: 'utf8' })).not.toThrow()

Environment if relevant (please complete the following information):

  • OS: Tested on Windows 10, but should affect all OSes
  • Node version 15.8.0, 14.15.4
  • Yarn version 2.4.0, current master
@clemyan clemyan added the bug Something isn't working label Feb 13, 2021
@yarnbot

This comment has been minimized.

@yarnbot yarnbot added the reproducible This issue can be successfully reproduced label Feb 13, 2021
@yarnbot

This comment has been minimized.

@clemyan
Copy link
Member Author

clemyan commented Feb 13, 2021

Hmm.. that wasn't the error I was expecting. Looks like some how the sherlock environment cannot directly link: to a file. I will edit the repro.

@yarnbot yarnbot added broken-repro The reproduction in this issue is broken and removed reproducible This issue can be successfully reproduced labels Feb 13, 2021
@yarnbot

This comment has been minimized.

@yarnbot yarnbot added reproducible This issue can be successfully reproduced and removed broken-repro The reproduction in this issue is broken labels Feb 14, 2021
@yarnbot
Copy link
Collaborator

yarnbot commented Feb 14, 2021

This issue reproduces on master:

Error: expect(received).not.toThrow()

Error name:    "Error"
Error message: "Command failed: node main.js

events.js:292
      throw er; // Unhandled 'error' event
      ^
Error: Cannot find module 'answer/linked.js'
Require stack:
- /tmp/tmp-23II8su6BG9fO0/main.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:880:15)
    at Function.Module._load (internal/modules/cjs/loader.js:725:27)
    at Module.require (internal/modules/cjs/loader.js:952:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/tmp/tmp-23II8su6BG9fO0/main.js:7:3)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
Emitted 'error' event on process instance at:
    at emitUnhandledRejectionOrErr (internal/event_target.js:545:11)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:358:9)
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/tmp/tmp-23II8su6BG9fO0/main.js' ]
}
"

      5 |   new Worker(__filename)
      6 | } else {
    > 7 |   require('answer/linked.js')
        |   ^
      8 | }
      9 | 

      at Object.<anonymous> (main.js:7:3)
      Emitted 'error' event on process instance at:
      at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26) {
        code: 'MODULE_NOT_FOUND',
        requireStack: [ '/tmp/tmp-23II8su6BG9fO0/main.js' ]
      }
      at evalmachine.<anonymous>:27:14
      at Object.<anonymous> (../../github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-0ac41999f0.zip/node_modules/expect/build/toThrowMatchers.js:81:11)
      at Object.throwingMatcher [as toThrow] (../../github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-0ac41999f0.zip/node_modules/expect/build/index.js:342:33)
      at module.exports (evalmachine.<anonymous>:27:93)
      at async /github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:13
      at async executeInTempDirectory (../../github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:16)
      at async Object.executeRepro (../../github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:12)
    at module.exports (evalmachine.<anonymous>:27:93)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async /github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:13
    at async executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:16)
    at async Object.executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:12)
    at async ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:25:38)
    at async ExecCommand.validateAndExecute (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-a57989414f.zip/node_modules/clipanion/lib/advanced/Command.js:161:26)
    at async Cli.run (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-a57989414f.zip/node_modules/clipanion/lib/advanced/Cli.js:74:24)
    at async Cli.runExit (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-a57989414f.zip/node_modules/clipanion/lib/advanced/Cli.js:83:28)

@clemyan
Copy link
Member Author

clemyan commented Feb 16, 2021

More investigation:

The core of the issue is how (the various ways of setting) the --require flag affects workers. So, I have done some tests:

cosnt tests = [
	/* 1 */ () => {
		const worker = new Worker(__filename)
	},
	/* 2 */ () => {
		const orig = process.execArgv
		process.execArgv = ['--require', './preload.js', ...process.execArgv]
		const worker = new Worker(__filename)
		process.execArgv = orig
	},
	/* 3 */ () => {
		const worker = new Worker(__filename, { execArgv: ['--require', './preload.js', ...process.execArgv] })
	},
	/* 4 */ () => {
		const orig = process.env.NODE_OPTIONS
		process.env.NODE_OPTIONS = `--require ./preload.js ${process.env.NODE_OPTIONS}`
		const worker = new Worker(__filename)
		process.env.NODE_OPTIONS = orig
	},
	/* 5 */ () => {
		const orig = process.env.NODE_OPTIONS
		process.env.NODE_OPTIONS = `--require ./preload.js ${process.env.NODE_OPTIONS}`
		const worker = new Worker(__filename, { env: SHARE_ENV })
		process.env.NODE_OPTIONS = orig
	},
	/* 6 */ () => {
		const worker = new Worker(__filename, { env: { NODE_OPTIONS: `--require ./preload.js ${process.env.NODE_OPTIONS}` } })
	},
]

where preload.js is simply

console.log('preload', process.pid, require('worker_threads').threadId)

Out of these, only 3 and 6 (explicitly passing execArgv and env.NODE_OPTIONS to the Worker constructor, respectively) works.

I also tested the equivalent invocations of child_process.fork (except SHARE_ENV since that has no equivalent).

cosnt tests = [
	/* 1 */ () => {
		const cp = fork('./foo.js')
	},
	/* 2 */ () => {
		const orig = process.execArgv
		process.execArgv = ['--require', './preload.js', ...process.execArgv]
		const cp = fork('./foo.js')
		process.execArgv = orig
	},
	/* 3 */ () => {
		const cp = fork('./foo.js', { execArgv: ['--require', './preload.js', ...process.execArgv] })
	},
	/* 4 */ () => {
		const orig = process.env.NODE_OPTIONS
		process.env.NODE_OPTIONS = `--require ./preload.js ${process.env.NODE_OPTIONS}`
		const cp = fork('./foo.js')
		process.env.NODE_OPTIONS = orig
	},
	/* 6 */ () => {
		const cp = fork('./foo.js', { env: { NODE_OPTIONS: `--require ./preload.js ${process.env.NODE_OPTIONS}` } })
	},
]

Out of these, all but 1 works. This show there is a discrepancy between how worker_threads and child_process handle process.execArgv and process.env.NODE_OPTIONS.

Question: Should we just handle it in Yarn (e.g. monkey-patch Worker constructor) or report this as a bug in Node?
Related: nodejs/node#29117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reproducible This issue can be successfully reproduced
Projects
None yet
Development

No branches or pull requests

2 participants