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

util.promisify does not work as expected with object methods #30344

Closed
simone-sanfratello opened this issue Nov 9, 2019 · 11 comments
Closed
Labels
question Issues that look for answers.

Comments

@simone-sanfratello
Copy link

  • Version: 12.11.1 and 13.1.0
  • Platform: Linux *** 4.15.0-66-generic Deprecate domains #75-Ubuntu SMP Tue Oct 1 05:24:09 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: util

Hi all

I'm having an issue with util.promisify, trying to apply it to an object method, as follow

const net = require('net')
const util = require('util')

async function start () {
  try {
    const server = net.createServer()
    const listen = util.promisify(server.listen)
    await listen(9123)
    console.log('started')
  } catch (error) {
    console.error(error)
  }
}

start()

output is

TypeError: Cannot read property '_handle' of undefined
    at Server.listen (net.js:1383:12)
    at internal/util.js:277:30
    at new Promise (<anonymous>)
    at internal/util.js:276:12
    at start (/home/simone/Desktop/util-promisify.js:8:11)
    at Object.<anonymous> (/home/simone/Desktop/util-promisify.js:15:1)
    at Module._compile (internal/modules/cjs/loader.js:945:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:962:10)
    at Module.load (internal/modules/cjs/loader.js:798:32)
    at Function.Module._load (internal/modules/cjs/loader.js:711:12)

because in net.js:1383 there is

if (this._handle) {

while util.promisify do

original.call(this, ...args, (err, ...values) => {

so this at execution of listen function is no more its own instance, but a new context.

Reading the documentation, this is not reported, so I'm opening this issue - that I suppose to be a bug.

In my opinion, a possible solution could be adding an optional arg instance to util.promisify, like

util.promisify(original, [instance])

then, after checked that instance is an object and instance[original] exists, could be

original.call(instance || this, ...args, (err, ...values) => {

it could be called as

util.promisify(server.listen, server)

and works as aspected.

If you agree with this solution, I'd be glad to do that.

@lpinca
Copy link
Member

lpinca commented Nov 9, 2019

You can use Function.prototype.call() on the function returned by util.promisify().

await listen.call(server, 9123);

or better use events.once(emitter, name) for your example.

@tniessen
Copy link
Member

tniessen commented Nov 9, 2019

Regarding your proposed solution: That would mean that you would have to create a new promisified function for each function and for each instance. I think @lpinca's suggestion is generally preferrable over that.

@tniessen tniessen changed the title util.promisify does not work as aspected with object methods util.promisify does not work as expected with object methods Nov 9, 2019
@simone-sanfratello
Copy link
Author

Please note that I'm not asking for help to solve my problem, I already did by writing my own promisify function.

I'm reporting that, imho, util.promisify does not work as aspected and there is no information about that in the documentation.

@lpinca
Copy link
Member

lpinca commented Nov 9, 2019

util.promisify does not work as aspected.

Why not? You are promisifying a function on net.Server prototype and call that function without context. It's the same of doing:

const listen = server.listen;

listen(9123, callback);

@tniessen tniessen added the question Issues that look for answers. label Nov 9, 2019
@tniessen
Copy link
Member

tniessen commented Nov 9, 2019

I agree with @lpinca, it is working as expected, that is just how JavaScript works.

Please note that I'm not asking for help to solve my problem, I already did by writing my own promisify function.

You can do that, or you can just use bind if you really want to make your promisifed functions specific to instances:

util.promisify(server.listen.bind(server))

@simone-sanfratello
Copy link
Author

simone-sanfratello commented Nov 9, 2019

In case, I prefer this solution

    const server = net.createServer()
    const listen = util.promisify(server.listen.bind(server))
    await listen(9123)

Don't you think this should be in the documentation?

@tniessen
Copy link
Member

tniessen commented Nov 9, 2019

That seems to be exactly what I suggested 👍

@simone-sanfratello
Copy link
Author

Submitting in the same time :)

@tniessen
Copy link
Member

tniessen commented Nov 9, 2019

Don't you think this should be in the documentation?

It is just how JavaScript works, but since it is apparently causing confusion, I would be okay with briefly mentioning this in the documentation.

@simone-sanfratello
Copy link
Author

Thank you

trivikr pushed a commit that referenced this issue Nov 21, 2019
Fixes: #30344

PR-URL: #30355
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 21, 2019
Fixes: #30344

PR-URL: #30355
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this issue Dec 1, 2019
Fixes: #30344

PR-URL: #30355
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 17, 2019
Fixes: #30344

PR-URL: #30355
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@jordanbtucker
Copy link

jordanbtucker commented Mar 31, 2020

You'd probably want something like this.

const server = net.createServer()
server.listen(9123)
await events.once(server, 'listening')

Otherwise, your app might hang if the listening event never fires due to an error, e.g. something's already listening on the same port.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants