util: fix inspecting of proxy objects by jasnell · Pull Request #6465 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

util

Description of change

In certain conditions (see #6464), inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself.

Also adds util.isProxy()

Fixes: #6464

/cc @bnoordhuis

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

CI is green LGTM

-1, the whole point of proxies is being transparent

the whole point of proxies is being transparent

That’s true for the programmatic use of proxies, but util.inspect comes with the intention of showing humans what is going on.

@addaleax +1 . Note that getters/setters are also generally intended to be transparent for most cases but util.inspect will produce output like {a: [Getter]} which avoids similar stack-depth side effects... e.g.

var m = {};
Object.defineProperty(m, 'a', {
  enumerable: true,
  get: function() {
    console.log(this);
    return 5;
  }
});
util.inspect(m);
  // Returns `{a: [Getter]}`

(@thealphanerd ... what are you doing up at 2am reviewing code I wrote at 1am? Sleep my friend! Sleep!)

If you export this, it becomes public API, and should be documented.

+1 ... Forgot to add the doc for it. Will add soon!
On Apr 29, 2016 6:29 AM, "Colin Ihrig" notifications@github.com wrote:

In lib/util.js
#6465 (comment):

@@ -785,6 +792,10 @@ exports.isPrimitive = isPrimitive;

exports.isBuffer = Buffer.isBuffer;

+function isProxy(p) {

return binding.isProxy(p);
+}
+exports.isProxy = isProxy;

If you export this, it becomes public API, and should be documented.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6465/files/3d4ff4311a3cf1ff756413756ffc859a663aba99#r61577439

@cjihrig ... updated! PTAL

New CI after update: https://ci.nodejs.org/job/node-test-pull-request/2432/

Unnecessary blank line.

@cjihrig ... done! :-)

@vkurchatkin ... +1 on it being semver-major (unfortunately).

@vkurchatkin ... would this be a bit more palatable if there were an option on util.inspect to turn off this special treatment of Proxy objects? e.g. util.inspect(proxyObj, {unwrapProxy: false});

This should probably be camelCase.

LGTM pending CI, but it should sit for a couple days.

would this be a bit more palatable if there were an option on util.inspect to turn off

@jasnell after some thinking, I agree with your reasoning. Having an option is nice, but in practice no one uses them

@cjihrig +1 for letting it sit a couple days. There's definitely no rush on it.
@vkurchatkin ... awesome.

In certain conditions, inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself. Also adds util.isProxy() Fixes: nodejs#6464

Squashed, new CI: https://ci.nodejs.org/job/node-test-pull-request/2435/

CI is green. Will land on monday if there are no objections. /cc @nodejs/ctc

Looks like this conflates two things:

fixing inspect (which shouldn't be major)
adding util.isProxy() (which we should not do given we don't even know what the hell do do with the existing functions: util: deprecate most of util.is*() #6235)

Please do not land in this state.

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

You should check that typeof value === 'object' && value !== null, I think?

Style nit: can you drop the blank line?

Ack

@bnoordhuis ... updated

I understand the logic but I don't think the 'necessary' is accurate; if you skipped adding proxy to the cache here, the algorithm would still work, only less efficiently. The next iteration would check the cache, miss, call .getProxyDetails() for the details array, and add the undefined return value to the cache a few lines below.

Necessary in the sense of avoiding an extraneous call to getProxyDetails... Which I would consider to be a bug since the point is to only check when we don't know for sure. ;)

LGTM

CI after nits.. seems fine to me if others are happy with the details: https://ci.nodejs.org/job/node-test-pull-request/2459/

@thealphanerd and @cjihrig ... still LGTY?

LGTM

Yes, LGTM

In certain conditions, inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself. Fixes: #6464 PR-URL: #6465 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Landed in ba6196f.
Thank you all.

In certain conditions, inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself. Fixes: #6464 PR-URL: #6465 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In certain conditions, inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself. Fixes: nodejs#6464 PR-URL: nodejs#6465 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 - Please see our blog post for more info on the security contents of this release: - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/ * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

jasnell added the util Issues and PRs related to the built-in util module. label Apr 29, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 29, 2016

mscdex removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 29, 2016

cjihrig reviewed Apr 29, 2016
View reviewed changes

vkurchatkin added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 29, 2016

cjihrig reviewed Apr 29, 2016
View reviewed changes

util: fix inspecting of proxy objects

86daa71

In certain conditions, inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself. Also adds util.isProxy() Fixes: nodejs#6464

jasnell force-pushed the inspect-proxy branch from ad9be3a to 86daa71 Compare April 29, 2016 17:59

bnoordhuis reviewed May 1, 2016
View reviewed changes

Address nits

842a8e4

bnoordhuis reviewed May 2, 2016
View reviewed changes

jasnell removed the wip Issues and PRs that are still a work in progress. label May 3, 2016

jasnell closed this May 3, 2016

This was referenced May 4, 2016

Propose v6.1.0 wrong base please ignore #6556

Closed

Propose v6.1.0 (Openssl update) #6557

Merged

yous mentioned this pull request May 23, 2016

Translate "Node v6.1.0 (Current)" nodejs/nodejs-ko#348

Merged

zbjornson mentioned this pull request Jan 11, 2017

console.log(proxy) invokes get trap with built-in Symbols #10731

Closed

kriscendobot mentioned this pull request Jul 12, 2026

design(inspect): @endo/inspect package + shim for portable, safe inspection endojs/endo-but-for-bots#715

Draft

+                      // for it to get proper formatting, and because
+                      // the target and handle objects also might be
+                      // proxies... it's unfortunate but necessary.
+                      proxyCache.set(proxy, undefined);

               function formatValue(ctx, value, recurseTimes) {
+                if (ctx.showProxy &&
+                    (typeof value === 'object' || typeof value === 'function')) {

util: fix inspecting of proxy objects by jasnell · Pull Request #6465 · nodejs/node · GitHub

Uh oh!

Uh oh!

util: fix inspecting of proxy objects - #6465

util: fix inspecting of proxy objects#6465jasnell wants to merge 6 commits intonodejs:masternodejs/node:masterfrom jasnell:inspect-proxyjasnell/node:inspect-proxyCopy head branch name to clipboard

Conversation

jasnell commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

MylesBorins commented Apr 29, 2016

Uh oh!

vkurchatkin commented Apr 29, 2016

Uh oh!

addaleax commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Apr 29, 2016

Uh oh!

vkurchatkin commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

Fishrock123 commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 2, 2016

Uh oh!

Fishrock123 commented May 2, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

MylesBorins commented May 3, 2016

Uh oh!

cjihrig commented May 3, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

Reviewers

Assignees

Labels

util: fix inspecting of proxy objects#6465
jasnell wants to merge 6 commits into
nodejs:masterfrom
jasnell:inspect-proxy

jasnell commented Apr 29, 2016 •
edited
Loading

Fishrock123 commented Apr 29, 2016 •
edited
Loading