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

Wrong behaviour of console.log in node 10 #34610

Closed
oscaretu opened this issue Aug 3, 2020 · 11 comments
Closed

Wrong behaviour of console.log in node 10 #34610

oscaretu opened this issue Aug 3, 2020 · 11 comments
Labels
console Issues and PRs related to the console subsystem.

Comments

@oscaretu
Copy link

oscaretu commented Aug 3, 2020

  • Version: 10.20.1
  • Platform: Windows 10 Home, 64 bit
  • Subsystem: Windows on Linux (WSL)

What steps will reproduce the bug?

If you execute this program with v10.20.1:

'use strict';

let dead = Buffer.alloc(1023, 'DEADBEEF', 'hex');

console.log("\n* The next one shows an 'n' suffix:\n");

console.log(dead.readBigUInt64BE(6) );

console.log("\n* The next one doesn't show an 'n' suffix:\n");
console.log('dead.readBigUInt64BE(6) [dec]: ', dead.readBigUInt64BE(6) );

console.log("\n => The only difference is that before the number a text was shown. Is right this behaviour?\n");

console.log("* But it shows the 'n' suffix in I invert the order of the two parameters:\n");
console.log(dead.readBigUInt64BE(6), " <= dead.readBigUInt64BE(6) [dec]\n");

you get

* The next one shows an 'n' suffix:

13758460224454254253n

* The next one doesn't show an 'n' suffix:

dead.readBigUInt64BE(6) [dec]:  13758460224454254253

 => The only difference is that before the number a text was shown. Is right this behaviour?

* But it shows the 'n' suffix in I invert the order of the two parameters:

13758460224454254253n ' <= dead.readBigUInt64BE(6) [dec]\n'

How often does it reproduce? Is there a required condition?

It appears always if I use the v10.20.1 version. It doesn't happen if I use versions v12.18.3 or v14.7.0

What is the expected behavior?

You shoudn't get this:

dead.readBigUInt64BE(6) [dec]:  13758460224454254253

but

dead.readBigUInt64BE(6) [dec]:  13758460224454254253n

(with a "n" suffix at the end of the number)

And you shouldn't get this:

' <= dead.readBigUInt64BE(6) [dec]\n'

but this (without single quotes, and with a new line instead of \n):

 <= dead.readBigUInt64BE(6) [dec]

@addaleax addaleax added the console Issues and PRs related to the console subsystem. label Aug 3, 2020
@ghost
Copy link

ghost commented Aug 3, 2020

@oscaretu thanks for the issue :)
Indeed. I'm able to reproduce this. The same problem is present in node 10.22.0, I've checked it.

node 12.18.3 which is the current Active LTS does not have this error though. I believe we'll need to check out the difference between the two and make the required changes.
I'll look into the source code a bit later, have no time for this right now.

@ghost
Copy link

ghost commented Aug 3, 2020

@addaleax can you assign this one to me?

@addaleax addaleax assigned ghost Aug 3, 2020
@addaleax
Copy link
Member

addaleax commented Aug 3, 2020

@iandrc Sure – just be prepared, I think this may be something that might intentionally have been changed as a semver-major commit (i.e. not backportable).

@ghost
Copy link

ghost commented Aug 3, 2020

@addaleax so, what should we do in such a case? Would we need to add a different logic for this one or leave "as is?"

@addaleax
Copy link
Member

addaleax commented Aug 3, 2020

@iandrc That depends on the commit, e.g. whether it’s partially backportable or not. But probably leave it as-is, yes.

@ghost
Copy link

ghost commented Aug 3, 2020

@addaleax

Currently, I'm not as familiar with node internals as I wish but... I've taken a look at the issue. Tbh, I didn't find any specific change that may cause it.

Yet, I observed one interesting thing. If we run the proposed code, we'll see the output @oscaretu observed:

console.log('dead.readBigUInt64BE(6) [dec]:  ', dead.readBigUInt64BE(6) );
// dead.readBigUInt64BE(6) [dec]: 13758460224454254253

However, if we'll add some formatting specifiers, it'll work just fine. For example, the lines:

console.log('dead.readBigUInt64BE(6) [dec]: %d', dead.readBigUInt64BE(6));
// or
console.log('dead.readBigUInt64BE(6) [dec]: %o', dead.readBigUInt64BE(6));

print the line you expect to see, namely:

dead.readBigUInt64BE(6) [dec]: 13758460224454254253n

I guess, the problem is somehow related to console.log()'s format specifiers. Still, not quite sure

@richardlau
Copy link
Member

richardlau commented Aug 3, 2020

Glancing through the changelog for 12, I'd guess #25046 (explicitly marked (dont-land-on-v10.x) and/or #23162 (marked semver-major, see also discussion in #29592) could be at play.

To set some expectations, Node.js 10.x is in maintenance now, which means there are no scheduled further updates planned (it will continue to get critical security fixes up until it's End-of-Life in April 2021). If there are small, low risk changes the Release WG may allow them to land on the v10.x-staging branch.

@ghost
Copy link

ghost commented Aug 3, 2020

@richardlau yeah, you're probably right. I missed those changes. Thanks for helping me out. Could you, please, explain how did you search? Because I have no idea how to search through commits more efficiently. Just curious.

Yeah, I believe that we won't change anything for node v10.x. I mean it's not marked dont-land-on-v10.x for no reason. This problem is solved in newer versions, and we've provided several options that may be helpful for @oscaretu.

So, could we close this one? What do you think?

@richardlau
Copy link
Member

I vaguely remembered #29592 (not specifics, just the general gist of it) and CTRL+F searched through https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md for console and then util. Probably not the most efficient way.

If those two PRs (or one of them) were the fix then it's highly unlikely we'd backport to 10.x.

@ghost
Copy link

ghost commented Aug 4, 2020

In this case, we, probably, can close the ticket.

@lundibundi
Copy link
Member

I don't think there is anything actionable left here as per the discussion above. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants