FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

errors: skip fatal error highlighting on windows by Hakerh400 · Pull Request #33132 · nodejs/node · GitHub

/ node Public

errors: skip fatal error highlighting on windows - #33132

Closed
Hakerh400 wants to merge 1 commit into
nodejs:masterfrom
Hakerh400:tty
ConversationCommits (1)ChecksFiles changed
Closed

errors: skip fatal error highlighting on windows#33132
Hakerh400 wants to merge 1 commit into
nodejs:masterfrom
Hakerh400:tty

Conversation

Hakerh400 commented Apr 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Apr 28, 2020

jasnell commented Apr 28, 2020

Copy link
Copy Markdown
Member

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 commented Apr 28, 2020

Copy link
Copy Markdown
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 commented Apr 29, 2020

Copy link
Copy Markdown
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 commented May 5, 2020

Copy link
Copy Markdown
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 reviewed May 5, 2020
View reviewed changes
Comment thread lib/internal/errors.js Outdated
BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 6, 2020

nodejs-github-bot commented May 6, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31184/

errors: skip fatal error highlighting on windows
9789cf6 
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 commented May 6, 2020

Copy link
Copy Markdown
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 commented May 6, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31186/

nodejs-github-bot commented May 9, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31248/

Hakerh400 commented May 17, 2020

Copy link
Copy Markdown
Contributor Author

friendly ping

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2020
errors: skip fatal error highlighting on windows
1c61914 
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 commented May 18, 2020

Copy link
Copy Markdown
Member

Landed in 1c61914 🎉

BridgeAR closed this May 18, 2020
Hakerh400 deleted the tty branch May 18, 2020 07:45

DerekNonGeneric commented May 18, 2020

Copy link
Copy Markdown
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
errors: skip fatal error highlighting on windows
a33e08e 
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
errors: skip fatal error highlighting on windows
62af877 
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
errors: skip fatal error highlighting on windows
5d80576 
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 mentioned this pull request Jun 9, 2020
Merged
codebytere mentioned this pull request Jun 28, 2020
Merged
QwireyInc pushed a commit to QwireyInc/node that referenced this pull request Apr 19, 2025
errors: skip fatal error highlighting on windows
aca07f4 
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/node#33132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uncaught error stack trace is misformatted on some platforms

6 participants

Back | FazBrowse Home | New Git URL