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

setTimeout cannot be util.promisify'ed #13654

Closed
gabmontes opened this issue Jul 12, 2018 · 9 comments
Closed

setTimeout cannot be util.promisify'ed #13654

gabmontes opened this issue Jul 12, 2018 · 9 comments

Comments

@gabmontes
Copy link

  • Electron Version: 1.8.4
  • Operating System (Platform and Version): macOS 10.13.5
  • Last known working Electron version: ?

Expected Behavior
require('util').promisify(setTimeout)(delay).then(callback) should work as per Node.JS 8.x docs.

Actual behavior
"callback" argument must be a function error is thrown inside promisified setTimeout.

To Reproduce
Execute this in the main process:

require('util').promisify(setTimeout)(0).then(console.log('works!'))

It will throw the mentioned error.

Additional Information
The problem seems to be that setTimeout[util.promisify.custom] is not defined. Therefore util.promisify tries to promisify setTimeout as a standard callback-last function but it is a special-case callback-first one.

See:
https://nodejs.org/api/timers.html#timers_settimeout_callback_delay_args
https://nodejs.org/api/util.html#util_custom_promisified_functions

Haven't tested in v2.x or higher yet. Is there plans to eventually backport a fix for this to the 1.x versions?

Thanks!!

@welcome
Copy link

welcome bot commented Jul 12, 2018

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

To help make it easier for us to investigate your issue, please follow the contributing guidelines.

@ghost
Copy link

ghost commented Jul 13, 2018

does electron 1.8.4 include a new enough node? You might want to use pify instead.

@pronebird
Copy link

This works in node 8.11.2, but electron 1.8.4 runs node 8.2.1 AFAIK. So maybe there is some issue with this.

@gabmontes
Copy link
Author

@pronebird you are correct. process.versions in shows the following:

{
  "http_parser": "2.7.0",
  "node": "8.2.1",
  "v8": "5.9.211.38",
  "uv": "1.13.1",
  "zlib": "1.2.11",
  "ares": "1.10.1-DEV",
  "modules": "57",
  "openssl": "1.0.2l",
  "electron": "1.8.4",
  "chrome": "59.0.3071.115",
  "atom-shell": "1.8.4"
}

And yes, Node.js v8.2.1 already supports custom promisifying setTimeout but that is not working on this Electron version.

@pronebird
Copy link

pronebird commented Jul 16, 2018

@gabmontes what happens if you print out setTimeout[util.promisify.custom]?

You know what, that's weird.

Broken in Electron v2.0.5 interactive shell (node v8.9.3):

$ electron -i
> setTimeout[util.promisify.custom]
undefined
> util.promisify(setTimeout)(1000).then(() => { console.log('hello'); }).catch(err => { console.log('err:', err); })
Promise {
  <pending>,
  domain: 
   Domain {
     domain: null,
     _events: { error: [Function: debugDomainError] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] } }
> err: TypeError: "callback" argument must be a function
    at setTimeout (timers.js:427:11)
    at /Users/pronebird/mullvad/mullvadvpn-app/node_modules/electron/dist/Electron.app/Contents/Resources/electron.asar/common/init.js:14:17
    at internal/util.js:227:26
    at repl:1:27
    at ContextifyScript.Script.runInContext (vm.js:59:29)
    at REPLServer.defaultEval (repl.js:242:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:442:10)
    at emitOne (events.js:116:13)

Works in node v8.11.2

$ node
> setTimeout[util.promisify.custom]
[Function]
> util.promisify(setTimeout)(1000).then(() => { console.log('hello'); }).catch(err => { console.log('err:', err); })
Promise {
  <pending>,
  domain: 
   Domain {
     domain: null,
     _events: { error: [Function: debugDomainError] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] } }
> hello

I think some of the custom promisify handlers haven't been properly set in Electron. It's also possible that it was a bug that was fixed in some patch release between node v8.9.3 and v.8.11.2. I don't really have any older node to verify that, but I think you have a clue how to workaround this for the time being.

@gabmontes
Copy link
Author

@pronebird Node.JS works fine. I have tested that in several versions as 8.2.1 (to match Electron 1.8.4) and 8.11.x (LTS).

But in Electron, the required property of setTimeout is just not set:

> promisify.custom
Symbol(util.promisify.custom)
> setTimeout[promisify.custom]
undefined

Since it is not set, setTimeout is promisified as a callback-last function and breaks because you pass a number to it and it is expecting the callback function there. The error you see in Electron 2.0.5 is the same as in 1.8.4.

@davidcornu
Copy link

This sounds really similar to #13458.

@gabmontes
Copy link
Author

@davidcornu for sure both issues have the same root cause!

@miniak
Copy link
Contributor

miniak commented Jul 29, 2018

@gabmontes Electron specific code is intercepting setTimeout, therefore setTimeout[util.promisify.custom] is being lost. I am fixing that in #13840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants