Problem with inspect and Node repl

[erkiesken]

I was trying to play with rxjs package in Node repl to compare it to v4 but kept on running into RangeError: Maximum call stack size exceeded errors.

Specifically when objects implement inspect method but its not meant for inspection then problems might occur.

For example with rxjs package:

> Rx = require("rxjs/Rx")
{ Subject: { [Function: Subject] create: [Function] },
  Observable:
…
> s = new Rx.Subject()
RangeError: Maximum call stack size exceeded
    at Subject.Observable (./node_modules/rxjs/Observable.js:21:24)
    at new Subject (./node_modules/rxjs/Subject.js:17:16)
    at Subject.lift (./node_modules/rxjs/Subject.js:28:23)

This is because Node's util.inspect (that repl uses to print expression return values) will try to call s.inspect but that will recurse and throw an error.

Fix is to disable customInspect option for util.inspect in Node repl as so:

> util = require("util");
> origInspect = util.inspect;
> util.inspect = obj => origInspect(obj, { customInspect: false })

Now the same thing as above won't throw:

> s = new Rx.Subject()
Subject {
  _isScalar: false,
  destination: undefined,
…

The customInspect option is documented here.

I'm not sure if there's anything Rx lib can do to prevent this. But at least a workaround notice in readme would be helpful.

I was using Node 5.6.0 and RxJS 5.0.0-beta2.

[kwonoj]

Am I understanding correctly this is naming collision between Rx.Observable.inspect to util.inspect?

[erkiesken]

It's not a naming collision. Node's util.inspect has a default way of showing some inspection output for objects, but when the object in question has a inspect method of its own then util.inspect will try to use that, in similar fashion to how toString() is called on objects.

But since Rx.Observable.inspect will return another observable, and util.inspect will call inspect again on this object, repeat this again and again and we end up with the call stack error above.

Mind you that that in real running code this problem doesn't appear, but it does in development workflow when using Node repl to test things out.

[kwonoj]

I see, thanks for clarification. It's not precise to say it's naming collision. As you said bit unsure if RxJS can do something about this other than add notes in document, but I may miss some possible solutions. Let's see if there's any suggestions around and if there isn't, I'll try to update document to include this topics.

[erkiesken]

One option would be to rename Rx.Observable.prototype.inspect to something else. It's somewhat similar to delayWhen in operation (and arguments) as I understand, so maybe rename inspect to inspectWhen?

[kwonoj]

Actually those naming pairs are more like relation between throttle to throttleTime, as inspect and inspectTime does. Course naming change will avoid this, but (purely personal opinion) I'm bit hesitate about those yet.

[erkiesken]

Node's util.inspect will invoke objects inspect with some arguments though:

> o = {}
{}
> o.inspect = function () { console.log("obj.inspect called", arguments); return "foo"; }
[Function]
> o
obj.inspect called { '0': 2,
  '1':
   { seen: [],
     stylize: [Function: stylizeWithColor],
     depth: 2,
     colors: true,
     showHidden: false,
     customInspect: true } }
foo

So one possible way is to do a arguments check in the Rx inspect method, for example this quick monkeypatch worked:

> origInspect = Rx.Subject.prototype.inspect;
[Function]
> Rx.Subject.prototype.inspect = function () { return arguments.length === 1 ? origInspect.apply(this, arguments) : this.toString(); }
[Function]
> new Rx.Subject()
[object Object]

But this would mean putting Node-specific workaround code into .ts files and all the compiled outputs.

[kwonoj]

But this would mean putting Node-specific workaround code into .ts files and all the compiled outputs.

: Yes, that's where my hesitation comes in, node (dev) specific changes. I don't expect util.inspect would be used for production code, is my understanding correct?

[erkiesken]

No, not automatically like in repl sessions.

[trxcllnt]

lol so node decided they own the word inspect. real great

[kwonoj]

@trxcllnt it seems so. I was able to dig up some old issues (nodejs/node-v0.x-archive#410 (comment)) and it's decided to not to rename it something like __inspect__ or similar.

[kwonoj]

/cc @Blesh also for visibility to conclude.

I'm still voting to add note into README, do not add specific logics for this.

[kennethlawrence]

I did want to mention that this problem extends outside of repl as well, as Node's version of console.log, console.info, & console.warn all use util.format, which itself invokes inspect. For example the following code will throw a RangeError:

'use strict'
const Rx = require('rxjs/Rx');

let o = Rx.Observable.of(1,2,3,4);
console.log(o);

Produces:

util.js:209
function formatValue(ctx, value, recurseTimes) {
                    ^

RangeError: Maximum call stack size exceeded
    at formatValue (util.js:209:21)
    at formatValue (util.js:221:13)
    at formatValue (util.js:221:13)
    ...

As above, this shouldn't be used in production code as console is not a standardized part of the language, but it is possible that developers may run into this on development code.

Personally I feel that this issue could be considered a bug with Node as opposed to RxJS, and wouldn't really expect any specific action within RxJS to deal with it. Perhaps it should be noted in the README as well for those left searching for solutions to RangeError bugs while developing.

[trxcllnt]

@tehnomaag @kennethlawrence from what I can tell, node's util.inspect passes a depth parameter to inspect.

@Blesh @kwonoj since our inspect takes a function, could we duck type a bit to check whether the argument is an integer, and return a string instead? I guess we could throw too, but not throwing seems friendlier. Since this is in an operator creation function, there won't be much meaningful performance degradation imo.

[benlesh]

... or we could just rename it.

[benlesh]

Yeah, given that the inspect name was bestowed upon these operators as a variation of the sample operators in #1053, I think we have carte blanche to just name them something else.

Any suggestions?

[trxcllnt]

@Blesh but I like inspect! :pouts:

[kennethlawrence]

@Blesh From looking at the documentation and playing around with inspect, perhaps audit and auditTime would be valid names.

Auditing is synonymous with inspecting, but repeating an audit when there is no change in information will not produce anything new. In this case the action could be described as "auditing for new information".

[trxcllnt]

@Blesh @kennethlawrence 👍 for either duck type checking in inspect or audit rename

[kwonoj]

Let me close this issue, by #1512 now inspect is renamed to audit. Please feel freely reopen if there's remaining issue.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.