lib: fixing isStackOverflowError to account for different JS engines #19705
Conversation
nodejs-github-bot
added
the
errors
Trott
added
the
author ready
|
Nit: Should update the comment starting on line 577 to reflect this change. |
lib/internal/errors.js
Outdated
| } | ||
| } | ||
|
|
||
| return err.name === 'RangeError' && err.message === MAX_STACK_MESSAGE; | ||
| return err.name === MAX_STACK_ERROR_NAME && err.message === MAX_STACK_ERROR_MESSAGE; |
There was a problem hiding this comment.
The linter failed because this line is now longer than 80 characters. You'll need to either split this across two lines or shorten the names of ALL_CAPS variables.
Especially since they're not constants, it may be more stylistically appropriate to camelCase them? That would shave off 6 characters, making the line exactly 80 characters long. Perfect! :-D
There was a problem hiding this comment.
Thanks, should be fixed up now.
There was a problem hiding this comment.
@Trott Hmm, I'm not sure about this... They are constants -- just lazily loaded constants.
Assumption that stack overflow exception has name == "RangeError" is v8-specific. Updated logic to dynamically capture error name when capturing error message.
mike-kaufman
force-pushed
the
mkaufman-fix-is-stack-overflow-for-node-chakracore
branch
from
9a19c0a
to
01b9afb
Compare
…ngines Remove extraneous space.
lib/internal/errors.js
Outdated
| @@ -572,7 +572,8 @@ function dnsException(err, syscall, hostname) { | |||
| return ex; | |||
| } | |||
|
|
|||
| let MAX_STACK_MESSAGE; | |||
| let MAX_STACK_ERROR_NAME; | |||
| let MAX_STACK_ERROR_MESSAGE; | |||
| /** | |||
| * Returns true if `err` is a `RangeError` with an engine-specific message. | |||
There was a problem hiding this comment.
This comment needs to be updated as well.
|
not sure why that node-test-commit check is failing. Nothing was obvious in logs. Is there some way to restart just that check? |
|
@mike-kaufman Yeah, this one is pretty tedious to track down and I can't tell what's wrong either. Certainly build related and not related to this change. From https://ci.nodejs.org/job/node-test-commit/17310/, scroll down to the bottom to see that node-test-commit-plinux is failing. Click through to those results (https://ci.nodejs.org/job/node-test-commit-plinux/16473/) and scroll to the bottom again. ppcle-ubuntu1404 is failing (https://ci.nodejs.org/job/node-test-commit-plinux/16473/nodes=ppcle-ubuntu1404/). From that page, click on Console Output (in the left nav) to go to https://ci.nodejs.org/job/node-test-commit-plinux/16473/nodes=ppcle-ubuntu1404/console and see that...hmmm, something is wrong but it's unclear what. Click "View as plain text" or "Full log" to get different views of the full log of the build. Searching through the results of that (which can take a while to load)...still not sure what went wrong. Ping @nodejs/build Meanwhile, let's re-run just node-test-commit-plinux: https://ci.nodejs.org/job/node-test-commit-plinux/16474/ |
|
(Re-run of single failing CI task was green so this is ready to land if there are no objections after 72 hours of being open.) |
|
Thanks @Trott! |
Assumption that stack overflow exception has name == "RangeError" is v8-specific. Updated logic to dynamically capture error name when capturing error message. PR-URL: nodejs#19705 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in cd5f353 |
Assumption that stack overflow exception has name == "RangeError" is v8-specific. Updated logic to dynamically capture error name when capturing error message. PR-URL: #19705 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Assumption that stack overflow exception has name == "RangeError" is
v8-specific. Updated logic to dynamically capture error name when
capturing error message.
make -j4 test(UNIX), orvcbuild test(Windows) passes