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

vm module timeout option not working in main process #26560

Closed
3 tasks done
Prinzhorn opened this issue Nov 18, 2020 · 4 comments
Closed
3 tasks done

vm module timeout option not working in main process #26560

Prinzhorn opened this issue Nov 18, 2020 · 4 comments
Labels
9-x-y 10-x-y 11-x-y bug 🪲 platform/linux stale status/confirmed A maintainer reproduced the bug or agreed with the feature

Comments

@Prinzhorn
Copy link
Contributor

Preflight Checklist

  • I have read the Contributing Guidelines for this project.
  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

  • Electron Version: v11.0.1
  • Operating System: Ubuntu 20.04
  • Last Known Working Electron version: None, I tried 6 through 11

I must assume this is a known issue, since the tests are disabled

"parallel/test-vm-timeout",

but I haven't found a mention in the issues. I also cannot find a list of differences between Electron and Node.js. Maybe this would be a great addition? E.g. the use of BoringSSL and all other things that are known to be different and potentially breaking when relying on Node.js compatibility. E.g. similar to this https://www.electronjs.org/docs/development/electron-vs-nwjs but "Differences between Node.js and Electron's Node.js fork".

Anyway, this is about the vm module:

Expected Behavior

I expect runInNewContexts timeout option of the vm module to be respected.

Actual Behavior

It never times out.

To Reproduce

console.log('runnin');

try {
  require('vm').runInNewContext('while (true);', {}, { timeout: 1 });
} catch (err) {
  console.error(err);
}

console.log('stoppin');

Electron:

$ npx electron@11 index.js
runnin
# Blocks forever

Node.js:

$ node index.js 
runnin
vm.js:131
      return super.runInContext(contextifiedObject, ...args);
                   ^

Error: Script execution timed out after 1ms
    at Script.runInContext (vm.js:131:20)
    at doWithTimeout (/home/alex/Projects/Bandsalat/src/issues/vm2-timeout/node_modules/vm2/lib/main.js:467:17)
    at VM.run (/home/alex/Projects/Bandsalat/src/issues/vm2-timeout/node_modules/vm2/lib/main.js:849:10)
    at Object.<anonymous> (/home/alex/Projects/Bandsalat/src/issues/vm2-timeout/index.js:5:24)
    at Module._compile (internal/modules/cjs/loader.js:1156:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1176:10)
    at Module.load (internal/modules/cjs/loader.js:1000:32)
    at Function.Module._load (internal/modules/cjs/loader.js:899:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47 {
  code: 'ERR_SCRIPT_EXECUTION_TIMEOUT'
}

Additional Information

Originally reported to the vm2 module patriksimek/vm2#307
The comments might help tracing this down patriksimek/vm2#307 (comment)

This is explicitly about the main process. I don't care about vm being removed from the renderer #26087

@Prinzhorn
Copy link
Contributor Author

Turns out it does work in a Worker Thread

const { Worker, isMainThread } = require('worker_threads');

if (isMainThread) {
  console.log('spawnin');
  new Worker(__filename);
} else {
  console.log('runnin');

  try {
    require('vm').runInNewContext('while (true);', {}, { timeout: 1 });
  } catch (err) {
    console.error(err);
  }

  console.log('stoppin');
}
 $ npx electron@11 worker.js
spawnin
runnin
Error: Script execution timed out after 1ms
    at Script.runInContext (vm.js:130:18)
    at Script.runInNewContext (vm.js:135:17)
    at Object.runInNewContext (vm.js:302:38)
    at Object.<anonymous> (/home/alex/Projects/Bandsalat/src/issues/vm2-timeout/worker.js:10:19)
    at Module._compile (internal/modules/cjs/loader.js:1152:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1173:10)
    at Module.load (internal/modules/cjs/loader.js:992:32)
    at Function.Module._load (internal/modules/cjs/loader.js:885:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at MessagePort.<anonymous> (internal/main/worker_thread.js:162:24) {
  code: 'ERR_SCRIPT_EXECUTION_TIMEOUT'
}
stoppin

I'm doing everything in a worker thread far away from the main process anyway. It's still a bug that the timeout does not work directly in the main process, but it doesn't concern me anymore.

@ckerr
Copy link
Member

ckerr commented Nov 18, 2020

Thanks for reporting this and for including a testcase!

Also, I like the idea of a "known differences" document but it's off-topic for this ticket. Could you file a separate ticket for that so that it can be tracked separately?

I must assume this is a known issue, since the tests are disabled

Looks like this test has been disabled since we first started running Node.js's test suite in our CI back in 1d06f67. At that time about 10% of the tests were failing. So this is a known issue in the sense that it's on the exclude list; but since it was one out of hundreds, I don't know if this one has had any specific discussion.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2022

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@github-actions github-actions bot added the stale label Oct 6, 2022
@Prinzhorn
Copy link
Contributor Author

Prinzhorn commented Oct 6, 2022

Turns out npx electron@11.0.0 index.js doesn't work but npx electron@11.5.0 index.js does (I didn't bisect the versions any further). So it turns out this has been fixed looooooooooooooooooooooooooooong ago, but this issue wasn't cross referenced.

The vm-timeout test was re-added here 5e1fbc9#diff-92a1788fffc3fc1f46ede1243c9ba69c6043c09f29bca54aa145b389836c7ff1L144

I don't know when exactly this was fixed, but 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9-x-y 10-x-y 11-x-y bug 🪲 platform/linux stale status/confirmed A maintainer reproduced the bug or agreed with the feature
Projects
None yet
Development

No branches or pull requests

2 participants