inspector: fix inspector::Agent::HasConnectedSessions

[helloshuangzi]

In Agent::HasConnectedSessions, return false if client_ is nullptr.
That's possible when inspector setting is skipped for embedding/addon scenarios.
It makes the code more robust at least, otherwise, crash will happen here.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[eugeneo]

It is possible to build Node without the inspector. Should that mode be used for embedding?

[helloshuangzi]

@eugeneo Thanks for the quick response!
Yes, I believe that helps for embedding, but I still think this change should be there to make the code more robust.
The problem also happens for addon users, who can't control the build mode of node.
Actually, I am working on re-implementing napajs worker::WorkerThreadFunc by node public API. There is no public API available now to set up inspector, and I don't want to enable inspector at the current stage.

[eugeneo]

I am still not comfortable with this fix, because it means the rest of the Node is unaware that the inspector might be missing. I assume the issue you are facing is because of WaitForInspectorDisconnect call in node.cc. Would it be possible not to call it instead? I think we should not reference env->inspector_agent at all if the Inspector is not properly setup.

[helloshuangzi]

Thanks for the suggestion, but I am not calling WaitForInspectorDisconnect.
Actually, the issue is from the below stack.

LoadEnvironment(&env);
-consoleCall
--InspectorEnabled(Environment* env)
---HasConnectedSessions()

[eugeneo]

Then it looks like console should not be pinging inspector when the inspector is n/a. IMHO, the best fix would still be to compile without the inspector altogether if it is not needed.

Anyway, I will not be blocking this PR.

[helloshuangzi]

napajs works as a node.js addon. As same as other addon usage, we can't control the building mode of node.
@eugeneo, please let me know if you have a better way for addon scenarios like ours.

[eugeneo]

@helloshuangzi try ./configure --without-inspector when building Node.

[helloshuangzi]

@eugeneo I know that flag, and it did help embedding scenarios if it works, but not for addon scenarios. Because the addon will depend on a self-built node to run it. It would be a significant drawback if an addon can't run with the official release version of node.js.

[addaleax]

IMHO, the best fix would still be to compile without the inspector altogether if it is not needed.

@eugeneo I think that’s not a viable long-term solution for embedders.

[eugeneo]

@addaleax inspector::Agent::Start should always be called if the inspector::Agent instance had been created (i.e. if the Node was built with the inspector support). I will work on that fix when I have some time.

As I said, I am not blocking this PR.

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/18396/

[helloshuangzi]

I noticed there were unsuccessful checks in the last CI, which can also be found for other PRs yesterday. I think it may got fixed by newly landed commits, so I just rebased it by the latest master. Please let me know if I should not do this after a collaborator started CI.

[eugeneo]

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

[addaleax]

Landed in 34ca9f3