util: fix inspecting of proxy objects #6465

jasnell · 2016-04-29T08:10:01Z

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

jasnell · 2016-04-29T08:10:42Z

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

MylesBorins · 2016-04-29T09:09:00Z

CI is green LGTM

vkurchatkin · 2016-04-29T09:14:32Z

-1, the whole point of proxies is being transparent

addaleax · 2016-04-29T12:44:02Z

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.

jasnell · 2016-04-29T13:20:53Z

@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]}`

jasnell · 2016-04-29T13:21:58Z

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

cjihrig · 2016-04-29T13:28:54Z

lib/util.js

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


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

jasnell · 2016-04-29T16:10:35Z

@cjihrig ... updated! PTAL

jasnell · 2016-04-29T16:11:17Z

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

cjihrig · 2016-04-29T16:17:07Z

doc/api/util.md

+
+```js
+const util = require('util');
+


Unnecessary blank line.

jasnell · 2016-04-29T16:38:40Z

@cjihrig ... done! :-)

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

jasnell · 2016-04-29T16:39:47Z

@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});

cjihrig · 2016-04-29T17:03:55Z

test/parallel/test-util-inspect-proxy.js

+require('../common');
+const assert = require('assert');
+const util = require('util');
+const process_util = process.binding('util');


This should probably be camelCase.

cjihrig · 2016-04-29T17:04:33Z

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

vkurchatkin · 2016-04-29T17:19:03Z

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

jasnell · 2016-04-29T17:50:16Z

@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

jasnell · 2016-04-29T18:00:22Z

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

jasnell · 2016-04-29T23:17:57Z

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

Fishrock123 · 2016-04-29T23:30:55Z

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.

Fishrock123 · 2016-04-30T20:42:54Z

Oh shoot. I agree defaulting to false makes sense for that.

jasnell · 2016-04-30T20:54:47Z

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

bnoordhuis · 2016-05-01T09:20:19Z

lib/util.js

@@ -241,6 +242,41 @@ function inspectPromise(p) {


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


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

Style nit: can you drop the blank line?

jasnell · 2016-05-01T13:42:34Z

@bnoordhuis ... updated

bnoordhuis · 2016-05-02T05:20:41Z

lib/util.js

+        // 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);


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. ;)

bnoordhuis · 2016-05-02T05:21:36Z

LGTM

Fishrock123 · 2016-05-02T13:37:15Z

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

jasnell · 2016-05-03T16:40:35Z

@thealphanerd and @cjihrig ... still LGTY?

MylesBorins · 2016-05-03T16:59:24Z

LGTM

cjihrig · 2016-05-03T17:02:04Z

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>

jasnell · 2016-05-03T17:10:19Z

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

doc/api/util.md

```js

const util = require('util');

Copy link

Contributor

cjihrig Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary blank line.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: fix inspecting of proxy objects #6465

util: fix inspecting of proxy objects #6465

jasnell commented Apr 29, 2016 •

edited

Loading

jasnell commented Apr 29, 2016

MylesBorins commented Apr 29, 2016

vkurchatkin commented Apr 29, 2016

addaleax commented Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

cjihrig Apr 29, 2016

jasnell Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

cjihrig Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

cjihrig Apr 29, 2016

cjihrig commented Apr 29, 2016

vkurchatkin commented Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

Fishrock123 commented Apr 29, 2016 •

edited

Loading

Fishrock123 commented Apr 30, 2016

jasnell commented Apr 30, 2016

bnoordhuis May 1, 2016

jasnell May 1, 2016

jasnell commented May 1, 2016

bnoordhuis May 2, 2016

jasnell May 2, 2016

bnoordhuis commented May 2, 2016

Fishrock123 commented May 2, 2016

jasnell commented May 3, 2016

MylesBorins commented May 3, 2016

cjihrig commented May 3, 2016

jasnell commented May 3, 2016

util: fix inspecting of proxy objects #6465

util: fix inspecting of proxy objects #6465

Conversation

jasnell commented Apr 29, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell commented Apr 29, 2016

MylesBorins commented Apr 29, 2016

vkurchatkin commented Apr 29, 2016

addaleax commented Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

Choose a reason for hiding this comment

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

Choose a reason for hiding this comment

cjihrig commented Apr 29, 2016

vkurchatkin commented Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

Fishrock123 commented Apr 29, 2016 • edited Loading

Fishrock123 commented Apr 30, 2016

jasnell commented Apr 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented May 2, 2016

Fishrock123 commented May 2, 2016

jasnell commented May 3, 2016

MylesBorins commented May 3, 2016

cjihrig commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented Apr 29, 2016 •

edited

Loading

Fishrock123 commented Apr 29, 2016 •

edited

Loading