zkat / pacote

programmatic npm package and metadata downloader (moved!)
https://github.com/npm/pacote
MIT License
280 stars 62 forks source link

A promise was created in a handler error #145

Closed milesj closed 6 years ago

milesj commented 6 years ago

This following error is consistently showing up when my team and I run npm install on one of our projects. It wasn't happening a couple weeks ago.

(node:58234) Warning: a promise was created in a handler at /Users/miles_johnson/.nvm/versions/node/v8.9.1/lib/node_modules/npm/node_modules/pacote/lib/fetchers/registry/tarball.js:39:25 but was not returned from it, see http://goo.gl/rRqMUw
    at ret (eval at makeNodePromisifiedEval (/Users/miles_johnson/.nvm/versions/node/v8.9.1/lib/node_modules/npm/node_modules/bluebird/js/release/promisify.js:184:12), <anonymous>:8:21)

I'm guessing because this line (https://github.com/zkat/pacote/blob/latest/lib/fetchers/registry/tarball.js#L43) is not returning. Seems that was last touched 2 months ago, which sort of aligns with this showing up for us.

Node: 8.9.1 NPM 5.8.0

fernan256 commented 6 years ago

@milesj did you have any update on that warning?

milesj commented 6 years ago

@fernan256 I haven't really dug into it further, but it's still happening.

fernan256 commented 6 years ago

It's really annoying, hope we have some answer from the maintainers.

zkat commented 6 years ago

I don't get why this is happening tbh

fernan256 commented 6 years ago

@zkat so you're looking why this is happening?

zkat commented 6 years ago

I did, it just doesn't make sense. All the promises I can see getting created have handlers attacked to them. foo.pipe() doesn't create a promise (that I know of?!), so I've no clue why Node is warning about this.

/cc @mylesborins Do you have any clue about this?

MylesBorins commented 6 years ago

pinging @benjamingr since this is using bluebird

benjamingr commented 6 years ago

When you create a promise in a then you should generally return it because otherwise it is possible it's not being listened to and can cause an unhandled rejection.

If you explicitly want to opt out of it you have two options

I recommend fixing the core issue (that there is a promise no one is listening to) though :)

benjamingr commented 6 years ago

@zkat fromManifest creates a promise in https://github.com/zkat/pacote/blob/latest/lib/fetchers/registry/tarball.js#L55 (when it does fetch) but does not return it which causes the warning.

In all fairness, that code is fine and the warning is probably a bit too eager since a stream whose continuation we can listen to is returned in that case, just return null from the then handler calling fromManifest would fix it.

zkat commented 6 years ago

Thanks! That seems like an easy enough fix.

@milesj @fernan256 do either of you want to PR that?

fernan256 commented 6 years ago

@zkat where is the handler that calls fromManifest?

redonkulus commented 6 years ago

@zkat @fernan256 I took a stab at it: https://github.com/zkat/pacote/pull/148

fernan256 commented 6 years ago

@zkat Question when you think the fix is gonna be merged and how can we get the change, is gonna be a new release of the library in npm? Thanks.

zkat commented 6 years ago

@fernan256 it'll be done when it's done.

zkat commented 6 years ago

you can thumbs down all you want, I'm still not going to ship it faster than it's gonna ship ;)

redonkulus commented 6 years ago

Lol you’re the author you can do whatever you want. I just thought your comment was funny. It’s just a warning so it’s not critical.

zkat commented 6 years ago

go answer my question in the PR :P

redonkulus commented 6 years ago

It’ll be done when it’s done

mofax commented 6 years ago

this problem showed up on me a few hours ago, is it done, or still waiting to be done?

zkat commented 6 years ago

I'm waiting for extra confirmation on the patch in the pr. I don't consider this urgent because it's just a warning only some folks receive, and there's no actual bug: this is just an overly eager automatic warning making unnecessary noise with valid code.

melroy89 commented 6 years ago

Getting the same warnings indeed: arning: a promise was created in a handler at /usr/lib/node_modules/npm/node_modules/pacote/lib/fetchers/registry/tarball.js

melroy89 commented 6 years ago

Sorry @zkat , I think I really underrated your valuable work regarding pacote! Since most of the people don't have any clue what is behind the scenes of npm. Thanks!

zkat commented 6 years ago

cool. In any case, this is fixed in pacote@8.1.1, and will be included with the next npm release (which should be next week, I think).

fernan256 commented 6 years ago

@zkat great job! Thanks for your work!