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

node 10 test release: util.inspect causing more issues #20180

Closed
devsnek opened this issue Apr 20, 2018 · 10 comments
Closed

node 10 test release: util.inspect causing more issues #20180

devsnek opened this issue Apr 20, 2018 · 10 comments
Labels
util Issues and PRs related to the built-in util module.

Comments

@devsnek
Copy link
Member

devsnek commented Apr 20, 2018

build: v10.0.0-test72549aa9cd

someone using discord.js pinged me about console.log hanging their process with the "Message" object. i assume this is related to brigear's change but its affecting the test build and moar data so i thought i would bring it up.

for debugging purposes the class is here: https://github.com/discordjs/discord.js/blob/master/src/structures/Message.js

once instantiated it is fairly deep and has a lot of circulars

/cc @BridgeAR @nodejs/util

@addaleax
Copy link
Member

Is there some kind of reproduction? I’m not sure how much one would be able to tell just from looking at the source code…

@devsnek
Copy link
Member Author

devsnek commented Apr 20, 2018

@addaleax sorry i don't have a lot of info. yeah this was kinda meant to be a reproduction since iirc the only other place it happened was inside npm internals. but also i just wanted to make people aware that the change to util.inspect still appears to be in v10 builds

@vsemozhetbyt
Copy link
Contributor

Was not the #20089 included in v10.0.0-test72549aa9cd?

@devsnek
Copy link
Member Author

devsnek commented Apr 20, 2018

if it was included then there's another bug here...

here's the screenshot they sent me

@vsemozhetbyt
Copy link
Contributor

Oops. I can still reproduce the #19405 with https://nodejs.org/download/test/v10.0.0-test72549aa9cd/

Either the revert was not successful, or there is another cause.

cc @jasnell

@vsemozhetbyt
Copy link
Contributor

Quick test on Windows:

node.v10.0.0-test72549aa9cd.exe "c:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js" install eslint@4.18.2
node.v10.0.0-test72549aa9cd.exe "c:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js" update

~5 min hanging with 100% CPU and 1.5 GB memory, I did not wait longer.

@vsemozhetbyt vsemozhetbyt added this to the 10.0.0 milestone Apr 20, 2018
@vsemozhetbyt vsemozhetbyt added the util Issues and PRs related to the built-in util module. label Apr 20, 2018
@Trott Trott added the confirmed-bug Issues with confirmed bugs. label Apr 20, 2018
@addaleax addaleax removed the confirmed-bug Issues with confirmed bugs. label Apr 20, 2018
@addaleax addaleax removed this from the 10.0.0 milestone Apr 20, 2018
@addaleax
Copy link
Member

I think it’s just an oversight in the test build name – the referenced commit, 72549aa, is on master, not v10.x-staging, and we didn’t land the revert on master (yet). We should probably change the test build name to v11.0.0-testX now?

@vsemozhetbyt
Copy link
Contributor

https://nodejs.org/download/rc/v10.0.0-rc.1/ is OK.

@devsnek Could the discord.js issue be checked with this last RC?

@MrJacz
Copy link

MrJacz commented Apr 20, 2018

I've just tested on rc.1, worked fine.

@Trott
Copy link
Member

Trott commented Apr 21, 2018

Seems like this can be closed (although maybe we should open an issue for bumping the nightly name to v11.0.0-testX but I'm not even sure where those are downloaded from TBH). Feel free to comment or re-open if you disagree.

@Trott Trott closed this as completed Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

5 participants