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

errors: skip fatal error highlighting on windows #33132

Closed
wants to merge 1 commit into from
Closed

errors: skip fatal error highlighting on windows #33132

wants to merge 1 commit into from

Conversation

Hakerh400
Copy link
Contributor

@Hakerh400 Hakerh400 commented Apr 28, 2020

Some consoles do not convert ANSI escape sequences to colors,
rather display them directly to the stdout. On those consoles,
libuv emulates colors by intercepting stdout stream and calling
corresponding Windows API functions for setting console colors.
However, fatal error are handled differently and we cannot easily
highlight them.

Fixes #29387


According to this, Windows fully supports ANSI escapes starting from v.1607 ("Anniversery Update", OS build 14393), so we could add additional check:

if (process.platform === 'win32') {
  const ver = os.release().split('.').map(a => +a);
  if (ver[0] !== 10 || ver[2] < 14393) {
    return originalStack;
  }
}

but given that #29387 (comment) uses build 17763, seems like that this test is not reliable.


Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Apr 28, 2020
@jasnell
Copy link
Member

jasnell commented Apr 28, 2020

Hmm... I understand with and agree with the general nature of this change but it still bothers me. Presumptively, this is an issue for more than just printing the error stack and I really dislike being forced to spread platform specific checks throughout the code as those become harder to maintain (or even keep track of) over time. I'm not sure I have a better suggestion right now but I'm reluctant to +1 this.

@BridgeAR
Copy link
Member

I am also reluctant to completely disable colors on fatal errors in Windows. The issue about the Windows version seems to be around differentiating Windows Server 2019 from regular Windows 10.

If there's any other possibility to detect for example if cmd.exe is used or not, we might at least use that in addition.

@Hakerh400
Copy link
Contributor Author

It's interesting how all tests passed, while none of them is modified. I actually see why - this change cannot be detected programmatically on Windows (or at least very hard).


I really dislike being forced to spread platform specific checks throughout the code as those become harder to maintain (or even keep track of) over time.

@jasnell. I hope we all realize this change is temporary. The actual fix is described in #29387 (comment), but it requires a lot of refactoring and also requires some changes in libuv. Given that this issue has been making troubles for a lot of Windows users for more than a year, wouldn't it be better to disable highlighting while we discuss how to actually resolve the issue?

I am also reluctant to completely disable colors on fatal errors in Windows.

@BridgeAR. I added additional check, PTAL. In general, there are no official Microsoft documentation regarding ANSI escapes support. There is a script for detecting the ANSI support, but it is not reliable. I tested it on different virtual machines with various console emulators and it does not work in all cases.

@Hakerh400
Copy link
Contributor Author

Any change requests here? If not, can we have this landed before 14.2.0 release (or at least before 14.3.0)?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 6, 2020
@nodejs-github-bot
Copy link
Collaborator

Some consoles do not convert ANSI escape sequences to colors,
rather display them directly to the stdout. On those consoles,
libuv emulates colors by intercepting stdout stream and calling
corresponding Windows API functions for setting console colors.
However, fatal error are handled differently and we cannot easily
highlight them.
@Hakerh400
Copy link
Contributor Author

Four tests were failing due to the fact that error object does not need to be an instance of Error constructor (and may not have stack present).

I updated the code and rebased onto master.
All tests seem to pass locally now. Can we have another CI?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Hakerh400
Copy link
Contributor Author

friendly ping

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2020
Some consoles do not convert ANSI escape sequences to colors,
rather display them directly to the stdout. On those consoles,
libuv emulates colors by intercepting stdout stream and calling
corresponding Windows API functions for setting console colors.
However, fatal error are handled differently and we cannot easily
highlight them.

PR-URL: nodejs#33132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in 1c61914 🎉

@BridgeAR BridgeAR closed this May 18, 2020
@Hakerh400 Hakerh400 deleted the tty branch May 18, 2020 07:45
@DerekNonGeneric
Copy link
Contributor

Glad this landed! I may be a bit late in asking, but would a TODO comment have been helpful to keep track of the fact that this is supposed to be a temporary fix and not a permanent solution?

codebytere pushed a commit that referenced this pull request May 21, 2020
Some consoles do not convert ANSI escape sequences to colors,
rather display them directly to the stdout. On those consoles,
libuv emulates colors by intercepting stdout stream and calling
corresponding Windows API functions for setting console colors.
However, fatal error are handled differently and we cannot easily
highlight them.

PR-URL: #33132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
Some consoles do not convert ANSI escape sequences to colors,
rather display them directly to the stdout. On those consoles,
libuv emulates colors by intercepting stdout stream and calling
corresponding Windows API functions for setting console colors.
However, fatal error are handled differently and we cannot easily
highlight them.

PR-URL: #33132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Jun 9, 2020
Some consoles do not convert ANSI escape sequences to colors,
rather display them directly to the stdout. On those consoles,
libuv emulates colors by intercepting stdout stream and calling
corresponding Windows API functions for setting console colors.
However, fatal error are handled differently and we cannot easily
highlight them.

PR-URL: #33132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Jun 9, 2020
@codebytere codebytere mentioned this pull request Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught error stack trace is misformatted on some platforms
6 participants