NODE_V8_COVERAGE does not fail gracefully when inspector is disabled

[addaleax]

• Version: master
• Subsystem: inspector

$ ./configure --without-intl
$ make -j4
$ NODE_V8_COVERAGE=/tmp/foo ./node -p 42

currently yields

internal/bootstrap/loaders.js:131
      mod = bindingObj[module] = getInternalBinding(module);
                                 ^

Error: No such module: profiler
    at internalBinding (internal/bootstrap/loaders.js:131:34)
    at setupCoverageHooks (internal/bootstrap/pre_execution.js:122:3)
    at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:21:7)

I feel like this should a) print some kind of helpful message and b) maybe only print a warning or so instead of failing to execute the script.

[Trott]

@bcoe

[shobhitchittora]

@addaleax I'd like to take a look at this. Up for the mentor-available thing. 😄

[addaleax]

@shobhitchittora Cool! As you can see from the stack trace, the function that sets up coverage is setupCoverageHooks in lib/internal/bootstrap/pre_execution.js, so that’s likely what you’d want to modify. There are also other examples of warnings being emitted in that file that you can take a look at.

As for contributing in general, https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md and https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md are probably helpful starting points. And if you run any trouble, feel free to ping me here or on Twitter or IRC (or generally feel free to ask a question in #node-dev on Freenode).

[shobhitchittora]

Thanks @addaleax for the code pointers. It's been some time since I pushed code here ( maybe in 2018 ), hope the guidelines are still the same.

Well! I see the docs and guidelines are much better organized now. Great work there!

[shobhitchittora]

I see that profiling logic has been recently moved to C++ land in #26874. Thinking as to why the reason the profiler binding is getting resolved here? Is something wrong with the bootstrapping here? @addaleax

[addaleax]

@shobhitchittora What’s wrong is that the profiler internal binding from C++ isn’t available when the inspector is disabled – that’s what’s causing the exception in the output that I posted.

One way (of multiple ones) to address this might be to watch out for such an exception in JS, and handle it appropriately rather than letting it crash the process.

[shobhitchittora]

Oh! Got it. I've added an try-catch block now and the output is as below -

>> NODE_V8_COVERAGE=/tmp/foo ./node -p 42
42
(node:12182) Warning: Profiler is not enabled.