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

util: recover from maximum call stack size #20725

Closed
wants to merge 2 commits into
base: master
from

Conversation

@BridgeAR
Member

BridgeAR commented May 14, 2018

Using util.inspect should still return values in case the maximum
call stack size is reached. This is important to inspect linked
lists and similar.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@BridgeAR

This comment has been minimized.

@addaleax

LGTM if it doesn’t affect the benchmarks negatively

@apapirovski

LGTM

@mscdex

I think at the very least this should be a separate option for util.inspect(). I would not expect to get back something I did not request. It should be all or nothing (meaning either the stack overflow error should occur or an error be thrown -- something to indicate the request could not be completed as asked).

If the user wants to explicitly opt-in to this behavior, that's fine because then they know what they could be getting into at that point.

@addaleax

This comment has been minimized.

Member

addaleax commented May 14, 2018

Here’s a benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/191/

But yes, I’m generally +1 on this PR. I don’t see the point of emitting a stack overflow error if we can avoid it at little cost. I don’t think setting depths to these high values makes sense in real-world applications anyway, but if somebody does that, then util.inspect() should yield some sensible output.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 16, 2018

Seems like TypedArrays take a minor hit of up to 4%. The other entries seem to be sough (I reran the benchmarks). I am going to play around with it to see if I am able to get that further down. I am a bit surprised that it has impact on those but not on e.g. nested objects.

About having depth levels like these in real world applications: This should indeed be highly unlikely as the output would mainly just be whitespace. I can not imagine that anyone would look at this and I can also not think of a use case where this would be useful programmatically. It would just not be useful anymore.

@TimothyGu

This comment has been minimized.

Member

TimothyGu commented May 20, 2018

Seems like TypedArrays take a minor hit of up to 4%.

I don't believe this should be a factor in deciding whether the PR should land. Agreed with @addaleax on user experience vs. cost.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 21, 2018

I had another look at this and found a solution that does not show the regression.

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

@BridgeAR

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 23, 2018

The benchmarks clearly show no regression (it will only effect very deeply nested objects - if at all).

I believe the user experience is better by returning a value instead of throwing. Inspecting any deeper will not provide a result that would still be really useful.

@mscdex are you strongly -1 on this?

@BridgeAR BridgeAR requested a review from nodejs/tsc May 24, 2018

@mcollina

LGTM. I think this will help new users.

@BridgeAR BridgeAR requested a review from nodejs/tsc May 25, 2018

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented May 26, 2018

I am -0.5 on this. In real-world applications that I've seen issues like this, even if the stack overflow doesn't happen, the process may run out of memory because of the complexity of the object. String concatenation in V8 is memory-expensive (each + takes up 24 bytes the last time I checked), not to mention all the whitespaces could result in a really large string which lives on the V8 heap, so this could trigger hard crashes instead of exceptions in those cases, and it could be much harder for users to understand what caused those crashes without post-mortem debugging.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 26, 2018

@joyeecheung the max stack size with be exceeded much much before the heap memory is. When I ran a different implementation that was recovering from the max stack size I was able to inspect a depth of almost 100.000. This will end inspection at a depth of ~1500 for the very simple case and earlier for more complex cases.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented May 26, 2018

@joyeecheung the max stack size with be exceeded much much before the heap memory is. When I ran a different implementation that was recovering from the max stack size I was able to inspect a depth of almost 100.000. This will end inspection at a depth of ~1500 for the very simple case and earlier for more complex cases.

IIUC, this ends the inspection early in the sense that it will return the formatted subtree to be concatenated later, it does not mean that the whole inspection process will end once the formatter finds one deep subtree in the object, so more memory will be consumed if the object is not skewed (containing multiple deep subtrees).

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 26, 2018

@joyeecheung that is correct but a very unlikely case. To have a object like this will likely cause problems one way or the other. The problem you speak about should be solved with #19994.

@ofrobots

I'm +1 on this as this is a debugging tool. Returning something is better than the stack-overflow.

@mhdawson

LGTM

@BridgeAR BridgeAR requested a review from nodejs/tsc May 28, 2018

@Trott

This comment has been minimized.

Member

Trott commented May 28, 2018

Just in case we start getting into vote-counting territory for the TSC (since it does seem unlikely that the impasse here will be resolved):

  • I'm an abstention. I defer to people who encounter these kinds of issues in real world applications more frequently than I do.
@Trott

This comment has been minimized.

Member

Trott commented May 28, 2018

(UPDATE: I'll be updating the preliminary vote counts as TSC people weigh in.)

Since this is at an impasse that seems unlikely to be resolved, I think calling for a vote at this time is appropriate. The way things stand now, as I see it at least:

There are 18 TSC members total, so this will require participation by at least another 2 TSC members to achieve a resolution.

@nodejs/tsc

lib/util.js Outdated
process.emitWarning(
'Inspection reached the maximum call stack size. Incomplete ' +
'inspected object returned.'
);

This comment has been minimized.

@addaleax

addaleax May 28, 2018

Member

I don’t think we should be printing this warning, it’s not a good idea to have side effects (especially unexpected) like stdio coming from util.inspect().

This comment has been minimized.

@BridgeAR

BridgeAR May 29, 2018

Member

Without the warning though, it would completely be silent and users would very likely not realize this at all. I guess it would not really be important / bad in this case but I believe it would be good to provide some kind of notification about it and I can not think of any alternative right now.

This comment has been minimized.

@addaleax

addaleax May 29, 2018

Member

@BridgeAR Can we add this to the output instead? That seems to make more sense to me, e.g. changing in the line below from [${constructor || tag || 'Object'}] to [${constructor || tag || 'Object'}: Maximum call stack size exceeded]?

This comment has been minimized.

@BridgeAR

BridgeAR May 29, 2018

Member

I think that is a viable solution. Should we maybe even be more verbose by writing e.g., [...: Inspection interrupted prematurely. Maximum call stack size exceeded.]?

I also think about adding the actually reached depth. Opinions about that?

This comment has been minimized.

@addaleax

addaleax May 29, 2018

Member

@BridgeAR Seems fine to me, yup.

I also think about adding the actually reached depth.

I wouldn’t expect it to be helpful a lot of the time (because that kind of nesting usually implies some kind of self-similar structure, so exact nesting levels are irrelevant), but it’s also not like it’s terrible information to include. 🤷

@danbev

danbev approved these changes May 29, 2018

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented May 29, 2018

@BridgeAR Have you considered @mscdex 's suggestion about making the recovery opt-in?

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 29, 2018

@joyeecheung I did consider it but we already have a lot of options for util.inspect and if it does not provide a substantial benefit I do not think we should add more. This PR should fix a rare edge case and makes sure the result will be somewhat usable. If it comes down to adding a new option or to add this change, I would rather close the PR again.

@Fishrock123

This seems like reasonable behavior to me

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Jun 6, 2018

I am not +1 but I would not block this. We can see how the users react to this in the wild and revert if necessary.

@targos

targos approved these changes Jun 6, 2018

LGTM but I would like Anna's suggestion to be implemented before landing

@Trott

This comment has been minimized.

Member

Trott commented Jun 6, 2018

To break the impasse, we had a vote in the TSC meeting today. This PR can land (assuming green CI).

Vote results:

Votes add up to 14 rather than 18 because 4 members were not involved in the vote:

  • @TimothyGu was not at the meeting to vote but is favorable to this PR above.
  • @joyeecheung did not vote that I saw in the meeting and may have dropped off by the time the vote happened. She was generally not favorable above, at least as I interpreted it, but not strongly so.
  • @MylesBorins was not at the meeting and has not commented on this issue as far as I can tell.
  • @gibfahn was not at the meeting and has not commented on this issue as far as I can tell.

I'll remove the tsc-agenda label.

BridgeAR added some commits May 14, 2018

util: recover from maximum call stack size
Using util.inspect should still return values in case the maximum
call stack size is reached. This is important to inspect linked
lists and similar.
@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Jun 11, 2018

I just pushed the requested change @addaleax

@BridgeAR

This comment has been minimized.

Dimissing the review due to the TSC vote.

@Trott

This comment has been minimized.

Member

Trott commented Jun 11, 2018

@targos

This comment has been minimized.

Member

targos commented Jun 24, 2018

@apapirovski

This comment has been minimized.

Member

apapirovski commented Jun 25, 2018

Landed in b26506b

apapirovski added a commit that referenced this pull request Jun 25, 2018

util: recover from maximum call stack size
Using util.inspect should still return values in case the maximum
call stack size is reached. This is important to inspect linked
lists and similar.

PR-URL: #20725
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit that referenced this pull request Jun 25, 2018

util: recover from maximum call stack size
Using util.inspect should still return values in case the maximum
call stack size is reached. This is important to inspect linked
lists and similar.

PR-URL: #20725
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment