Skip to content
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

Wrap custom inspect hook in try/catch within util.js #10587

Closed
skbolton opened this issue Jan 3, 2017 · 4 comments · May be fixed by O330oei/node#4 or O330oei/node#11
Closed

Wrap custom inspect hook in try/catch within util.js #10587

skbolton opened this issue Jan 3, 2017 · 4 comments · May be fixed by O330oei/node#4 or O330oei/node#11
Labels
util Issues and PRs related to the built-in util module.

Comments

@skbolton
Copy link

skbolton commented Jan 3, 2017

  • 7.0.0:
  • Darwin Kernel Version 16.4.0:
  • util module:

I was diagnosing an issue where calls to console.log(require.cache) was giving me lots of problems. After debugging for a while I think I found the problem. The module had an inspect function on it and that was being called when the console.log was formatting the output. I found this code in utils.js starting at line 342

// Provide a hook for user-specified inspect functions.
  // Check that value is an object with an inspect function on it
  if (ctx.customInspect && value) {
    const maybeCustomInspect = value[customInspectSymbol] || value.inspect;

    if (typeof maybeCustomInspect === 'function' &&
        // Filter out the util module, its inspect function is special
        maybeCustomInspect !== exports.inspect &&
        // Also filter out any prototype objects using the circular check.
        !(value.constructor && value.constructor.prototype === value)) {
      let ret = maybeCustomInspect.call(value, recurseTimes, ctx);

      // If the custom inspection method returned `this`, don't go into
      // infinite recursion.
      if (ret !== value) {
        if (typeof ret !== 'string') {
          ret = formatValue(ctx, ret, recurseTimes);
        }
        return ret;
      }
    }
  }

Wouldn't it be a good idea to wrap the call to maybeCustomInspect in a try/catch so that if library authors don't realize this naming collision exists and an error is thrown that the native formatValue is used?

let ret
try {
  ret = maybeCustomInspect.call(value, recurseTimes, ctx);
} catch () {
  ret = formatValue(ctx, ret, recurseTimes);
}
if (typeof ret !== 'string') {
  ret = formatValue(ctx, ret, recurseTimes)
}
@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Jan 3, 2017
@addaleax
Copy link
Member

@skbolton Sorry for the lack of follow-up from our side here. Do you remember which module this is about? If I understood the issue correctly, the best way to address this would be to add an module.exports[util.inspect.custom] = function() { return this }; function to the module in question (which is preferred over .inspect() in Node >= 6).

I am not sure a catch-all try { … } block is the best approach here … in the past, it was usually a sign of a real bug for me when a custom inspect() method threw an error.

@Trott
Copy link
Member

Trott commented Aug 6, 2017

Should this remain open?

@skbolton
Copy link
Author

skbolton commented Aug 6, 2017

Well I made a change in the library to no longer have an inspection method on it. But this is one of those things that authors wouldn't even realise is there

@apapirovski
Copy link
Member

apapirovski commented Apr 13, 2018

This is documented and using .inspect is deprecated. I'm not sure there's much more that can be done (until we eventually remove .inspect support and only use the symbol).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
5 participants