Ensure node-based debug adapters spawn same node exec as Theia #5492

perspectivus1 · 2019-06-17T10:22:14Z

Signed-off-by: Eyal Barlev eyal.barlev@sap.com

Sometimes the container/machine running Theia has more than one node executable available.

The node executable used to run Theia should also be used to run node-based debug adapters; whereas, another node executable can be used to run/debug user applications.

Debug adapters specify their runtime in their corresponding package.json file (e.g. link):

"contributes": {
  "debuggers": [
    {
       "runtime": "node"

The debug adapter typically does not specify the full path of the node executable, which means that Theia uses the first node executable on the PATH environment variable to spawn the debug adapter. However, this might not be the same node executable that was used to run Theia.

For us at SAP, this is critical, as we expose node executables to the Theia container, and those executables might run remotely on other containers that have no access to the debug adapter code (which resides on the Theia container).

This pull request ensures that if a debug adapter doesn't specify a fully qualified node runtime, Theia would spawn the debug adapter using the same node executable that was used to launch Theia.

This change should not adversely affect existing flows, but rather add support for the requirement detailed above.

Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

akosyakov · 2019-06-17T14:37:53Z

We've discussed something like that for TS language servers already. cc @marcdumais-work @kittaakos

I believe the proper way there and here is to use child_process.fork instead of guessing node executable from args. It's designed to start new processes with exactly the same node which is used to run Theia.

Why guessing would be better? @tolusha What is your take on it?

akosyakov · 2019-06-17T14:39:22Z

See VS Code use fork as well: https://github.com/microsoft/vscode/blob/466c132f5b4362b3be485a1f7a624b3a7316ddb8/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L186-L205

perspectivus1 · 2019-06-18T08:11:20Z

I tried your proposed approach here. This indeed ran the debug adapter using child_process.fork(), but debugging user code kept failing with this error message:

Starting inspector on 127.0.0.1:24166 failed: address already in use

Do you know why this is happening?

Note that the node's API documentation specifically states this:

child_process.fork() will spawn new Node.js instances using the process.execPath of the parent process.

tolusha · 2019-06-18T08:13:58Z

I personally stand for fork

akosyakov · 2019-06-18T08:22:12Z

@perspectivus1 Can you open an alternative PR with fork that we can have a look as well?

perspectivus1 · 2019-06-18T08:53:08Z

I created an alternative PR (#5508), with a fork() implementation (that fails to debug user code).

kittaakos · 2019-06-28T14:37:28Z

This won't work when running from electron, as it will spawn/fork a new electron process. Also, we have to generalize it and reuse for the TS and JSON LSs.

See: #5385

akosyakov · 2019-07-10T08:14:01Z

We should sort out Electron before merging.

Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

perspectivus1 · 2019-07-17T10:21:07Z

@akosyakov, I fixed the Electron case.

@kittaakos, there's a simple change that I have not pushed, that when running Electron would spawn the debug adapter as a node process:

In file packages\debug\src\node\debug-adapter-factory.ts:

        const options = { stdio: ['pipe', 'pipe', 2], env: environment.electron.runAsNodeEnv() };

Should I add it to this pull request?

kittaakos · 2019-07-17T11:26:06Z

packages/debug/src/node/debug-adapter-factory.ts

@@ -66,6 +68,12 @@ export class LaunchBasedDebugAdapterFactory implements DebugAdapterFactory {
        const isForkOptions = (forkOptions: RawForkOptions | any): forkOptions is RawForkOptions =>
            !!forkOptions && !!forkOptions.modulePath;

+        if (!isForkOptions(executable)) {
+            if ((executable as DebugAdapterSpawnExecutable).command === 'node' && !environment.electron.is()) {


From the description:

The node executable used to run Theia should also be used to run node-based debug adapters

This won't be the case if you run in electron.

Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

perspectivus1 · 2019-07-17T12:44:05Z

packages/debug/src/node/debug-adapter-factory.ts

+        if (!isForkOptions(executable)) {
+            if ((executable as DebugAdapterSpawnExecutable).command === 'node') {
+                if (environment.electron.is()) {
+                    options.env = environment.electron.runAsNodeEnv();


@kittaakos, is this what you meant?

It looks much better now, logically it is correct.

Can we move this Node.js-specific customization to the process-factory?
What's your take on that, @akosyakov?

I wonder why execPath is used only in else branch.

Can we move this Node.js-specific customization to the process-factory?
What's your take on that, @akosyakov?

Do you mean if command === process.execPath? for node i don't think so, it is not the same

I don't think this code should use process factory at all, but it is another issue. User processes (running in user dev env like terminals and task) should be created by a factory, all other should be created vis node js APIs.

Can we move this Node.js-specific customization to the process-factory?

So, should I move the implementation elsewhere?

execPath contains the full path of the Electron executable; whereas we want to spawn Electron's embedded node executable.

I am not sure if it is true. But please correct me if I am wrong.
If I have Node.js v10.15.3 available from the PATH and I start a Theia-based electron application which comes with electron 3.1.7 (as of today), then I will run with a different Node.js version than I am expecting.

Akoss-MacBook-Pro:theia akos.kitta$ npx electron -i > console.log(process.versions.node) 10.2.0 undefined > .exit

Does this make sense?

we want to spawn Electron's embedded node executable.

This part is not true, I think.

When Theia runs in node, we can control the node executable that is used to spawn the debug adapter, because we can infer it from process.execPath.

When Theia runs in Electron, process.execPath contains a full path to the electron executable. Therefore, we have 2 choices:

Spawn a node process using the first node executable found in the PATH environment variable.

Fork an electron process using the ELECTRON_RUN_AS_NODE environment variable, which will use Electron's embedded node.

However, the current implementation of AbstractVSCodeDebugAdapterContribution.provideDebugAdapterExecutable() creates a DebugAdapterExecutable that causes Theia to spawn, rather than fork debug adapters.

In other words, in the case of a node-based debug adapter, the code above always spawn a node process, regardless of whether Theia runs in Electron or in node and regardless of the value of the ELECTRON_RUN_AS_NODE environment variable.

If this is the desired behavior, then this PR fulfills the requirement.

If we wish to use Electron's embedded node, I will have to modify PR #5508.

Please advise

Forking is a proper solution. It is the same way as VS Code does it. I would be in favor of closing this PR and modifying #5508.

…d not fork) Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

akosyakov · 2019-07-25T08:18:02Z

Closing it in favor of #5508

Ensure node-based debug adapter spawn same node as Theia

5c7fbc8

Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

akosyakov requested review from tolusha, marcdumais-work and kittaakos June 17, 2019 14:38

perspectivus1 mentioned this pull request Jun 18, 2019

Ensure node-based debug adapters spawn same node exec as Theia #5508

Merged

tolusha approved these changes Jun 18, 2019

View reviewed changes

akosyakov added debug issues that related to debug functionality electron issues related to the electron target labels Jul 10, 2019

perspectivus1 added 2 commits July 17, 2019 12:57

Merge https://github.com/theia-ide/theia

ff1f50f

Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

Fixed spawning debug processes in Electron

3d01560

Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

kittaakos reviewed Jul 17, 2019

View reviewed changes

Spawn Node.js-based debug adapter using Electron's embedded Node.js

f8d930a

Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

perspectivus1 commented Jul 17, 2019

View reviewed changes

removing use of runAsNodeEnv(), as we always spawn debug adapters (an…

452ec24

…d not fork) Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>

akosyakov closed this Jul 25, 2019

veredcon mentioned this pull request Mar 8, 2020

Ensure node-based debug adapters spawn same node exec as Theia #7294

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure node-based debug adapters spawn same node exec as Theia #5492

Ensure node-based debug adapters spawn same node exec as Theia #5492

perspectivus1 commented Jun 17, 2019

akosyakov commented Jun 17, 2019 •

edited

Loading

akosyakov commented Jun 17, 2019

perspectivus1 commented Jun 18, 2019

tolusha commented Jun 18, 2019

akosyakov commented Jun 18, 2019

perspectivus1 commented Jun 18, 2019

kittaakos commented Jun 28, 2019 •

edited

Loading

akosyakov commented Jul 10, 2019

perspectivus1 commented Jul 17, 2019

kittaakos Jul 17, 2019

perspectivus1 Jul 17, 2019

kittaakos Jul 17, 2019

akosyakov Jul 18, 2019

akosyakov Jul 18, 2019

kittaakos Jul 18, 2019

perspectivus1 Jul 18, 2019

kittaakos Jul 18, 2019

kittaakos Jul 18, 2019 •

edited

Loading

perspectivus1 Jul 18, 2019

akosyakov Jul 19, 2019

akosyakov commented Jul 25, 2019

Ensure node-based debug adapters spawn same node exec as Theia #5492

Ensure node-based debug adapters spawn same node exec as Theia #5492

Conversation

perspectivus1 commented Jun 17, 2019

akosyakov commented Jun 17, 2019 • edited Loading

akosyakov commented Jun 17, 2019

perspectivus1 commented Jun 18, 2019

tolusha commented Jun 18, 2019

akosyakov commented Jun 18, 2019

perspectivus1 commented Jun 18, 2019

kittaakos commented Jun 28, 2019 • edited Loading

akosyakov commented Jul 10, 2019

perspectivus1 commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Jul 25, 2019

akosyakov commented Jun 17, 2019 •

edited

Loading

kittaakos commented Jun 28, 2019 •

edited

Loading

kittaakos Jul 18, 2019 •

edited

Loading