GH-815: Replace `child_process` with the `Process` API in the LS backend contribution #841

kittaakos · 2017-11-15T21:07:48Z

Closes: #815

hexa00 · 2017-11-16T00:39:47Z

packages/process/src/node/process.ts

-    set killed(killed: boolean) {
-        /* readonly public property */
-    }
-


can this be ommited ?
what happens on process.killed = boolean ?

process.killed = boolean

That is a compiler error. Having a property getter is equivalent to a readonly property from the client code's point of view. Let me know if you meant something else.

OK good :) that's what I wanted

hexa00 · 2017-11-16T00:42:07Z

packages/process/src/node/process-manager.ts

 import { Process } from './process';
 import { Emitter, Event } from '@theia/core/lib/common';
+import { ILogger } from '@theia/core/lib/common/logger';



Would be nice to have these changes in the previous commit

hexa00 · 2017-11-16T00:43:14Z

packages/process/src/node/process-manager.ts

-        this.id++;
-        return id;
+        this.processes.set(++this.id, process);
+        return this.id;


I just wanted the id to start at 0 .. you mind if if is -1 then ?

I do not understand what you mean. Could you please explain this to me?

I mean doing ++this.id from this.id = 0... will make the first process have id 1 instead of 0

Got it!

you mind if if is -1 then

-1 confused me. I will restore the original behavior. Thanks for the reminder!

this.processes.set(this.id++, process); return this.id

This would work actually :) with this.id = 0 to start.. so less confusing

Strangely, changing to this.id++ breaks the Terminal Backend Contribution is data received from the terminal ws server test. I have to check why.

hexa00 · 2017-11-16T00:50:23Z

packages/process/src/node/raw-process.ts

    readonly output: stream.Readable;
    readonly errorOutput: stream.Readable;
-    protected process: child.ChildProcess;
+    // XXX: Do we have to make this public? How to attach additional listeners to the underlying process then?
+    readonly process: child.ChildProcess;


I did not want to shadow the process to allow direct API access to it since I wasn't sure yet what was needed.

I think it may be better to leave it public in case a user wants to do something non trivial with it and we don't want to wrap the whole API in raw-process.

Not sure what you mean by attaching additional listeners ?

be better to leave it public

I agree. Otherwise, I could not listen on errors for instance here. One can check why stderr is not available on the ITerminal. I hope this answers your last question about the listeners too.

hexa00 · 2017-11-16T00:56:12Z

Looks good the refactoring of process/terminal :) thx

hexa00 · 2017-11-16T12:22:47Z

BTW I have not checked properly but it seems a bit weird to me that we have to manually send stdin to the terminal since it spawns the process and binds stdout/stderr to it...

Maybe we don't have to do this and it's node by node-pty already?

hexa00 · 2017-11-16T12:24:52Z

packages/core/src/node/backend-application.ts

+        // Handles `Ctrl+C`.
+        process.on('SIGINT', () => this.onStop());
+        // Handles `kill pid`
+        process.on('SIGUSR1', () => this.onStop());


kill pid sends SIGTERM, not SIGUSR...

Are you sure? I am not, after all. See: https://stackoverflow.com/questions/14031763/doing-a-cleanup-action-just-before-node-js-exits

Yes I'm sure, this answer is wrong. See https://linux.die.net/man/1/kill
The default signal for kill is TERM

SIGURS are user-defined:

SIGUSR1 30,10,16 Term User-defined signal 1 SIGUSR2 31,12,17 Term User-defined signal 2

So we should not exit on those see: http://man7.org/linux/man-pages/man7/signal.7.html

kittaakos · 2017-11-16T12:29:30Z

Maybe we don't have to do this and it's node by node-pty already?

You're the expert on this topic.

hexa00 · 2017-11-16T12:33:16Z

packages/languages/src/node/language-server-contribution.ts

+        this.spawnedProcessIds
+            .map(id => this.processManager.get(id))
+            .filter(process => process)
+            .forEach(process => this.processManager.delete(process!));


Should we wait for the processes to be killed before exiting ?

We can't be sure that just sending SIGTERM here will kill them...

Yes, each call must be synchronous. https://nodejs.org/api/process.html#process_event_exit

I mean the call to kill I think is synchronous... but the signal handler of the processes is not...
So you send kill to all .. the main process exists.. but the childs will live on until they've handled the SIGTERM.

And if they don't exit properly with SIGTERM you will have lost them.
So maybe we should wait for their exit event before ending this operation.

LS processes should exit automatically when the parent process is not alive, look for InitializeParams.processId comment in the lsp. If it does not happen it means that the worker process is alive. One should check it and if it is the case what the forked worker does not exit when the master exits, then changes in this pr won’t help since the worker is still running and the proper fix will be to explicitly stop running workers on exit in the master process.

LS processes should exit automatically when the parent process is not alive

That is not happening: #815 (comment)

the worker process is alive

I cannot see other Theia processes.

You mean, in general, any LS started from a jar?

Or we use outdated java ls, I can see in their code handling of this case: https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/LanguageServer.java

But the screenshot (gif whatever) I have attached to this thread does not do anything with the Java LS. It starts the Yang LS.

I see, I thought lsp4j or Xtext ls supports it, but cannot find such code there. Btw the lsp says that shutdown + exit requests should be sent to shutdown the server.

eclipse/xtext-core#552 - a bug for it in Xtext

hexa00 · 2017-11-16T12:51:30Z

You're the expert on this topic.

OK so I've verified this a bit and looking at the code for linux:
https://github.com/Tyriar/node-pty/blob/e9c790871b55c8fe9920409f59fb12360adda999/src/unix/pty.cc#L679

      dup2(slave, 0);
      dup2(slave, 1);
      dup2(slave, 2);

Stdin/stdout/stderr are mapped to the slave part of the pseudo terminal so anything writting to these outputs will appear. (And by default it's echo mode is on)

So you don't need to manually write what is coming from stdin or stdout.

See: http://rachid.koucha.free.fr/tech_corner/pty_pdip.html#Introduction_to_pseudo-terminals for more background info on ptys

akosyakov · 2017-11-16T15:16:33Z

packages/languages/src/node/language-server-contribution.ts

    abstract readonly id: string;
    abstract readonly name: string;
+    protected readonly spawnedProcessIds: number[];


I wonder processes can exit even if the worker is still running, then they should be removed from this list. Maybe we can use disposable pattern instead of onStop callback, here disposable collection will be used which is returned as a disposable by start method.

While this will change how the processes are disposed they will still be in the disposable list , even if they exited no ?

However this doesn't matter much.. if it has exited already the processManager will just ignore it and since ids are unique and not reused this is safe.

I thought that will be removed on exit by calling dispose on disposable returned from collection.push.

If you mean to make the Process disposable and that it disposes of itself on exit() that won't work since you can't be certain that all the event listeners on the object have been called once you dispose of it in your exit() handler.

Also it complicates things a lot because other components could have a reference to that process and try to call invalid methods on it now that it is disposed....

Or maybe I'm missing something?

Ok, I only meant to consider an alternative api for onStop: use disposable as a return type for onStart. If it makes things complicated I am fine with the current state like now.

kittaakos · 2017-11-20T13:20:03Z

Please review.

akosyakov · 2017-11-20T16:13:59Z

packages/languages/src/node/language-server-contribution.ts

    abstract start(clientConnection: IConnection): void;

+    stop(): void {
+        while (this.disposables.length > 0) {


if disposables is DisposableCollection, then you can just call dispose here

akosyakov · 2017-11-20T16:17:28Z

packages/languages/src/node/language-server-contribution.ts

-        return serverProcess;
+    protected spawnProcess(command: string, args?: string[], options?: cp.SpawnOptions): RawProcess {
+        const rawProcess = this.processFactory({ command, args, options });
+        this.disposables.push({


disposable should be removed if the process exits before stop. DisposableCollection.push returns Disposable to do it

akosyakov · 2017-11-20T16:18:02Z

packages/languages/src/node/language-server-contribution.ts

+            dispose: () => {
+                const process = this.processManager.get(rawProcess.id);
+                if (process) {
+                    this.processManager.delete(process);


and it happens automatically when the process exits, right?

No it happens when you stop the lsp.
The process has no auto cleanup mecanism

akosyakov · 2017-11-20T17:34:19Z

packages/process/src/node/raw-process.ts

+    readonly input: stream.Writable;
+    readonly output: stream.Readable;
+    readonly errorOutput: stream.Readable;
+    // XXX: Do we have to make this public? How to attach additional listeners to the underlying process then?


i think public is fine, why will we want to abstract clients from node.js apis?

akosyakov · 2017-11-20T17:41:05Z

packages/languages/src/node/language-server-contribution.ts

-        serverProcess.stderr.on('data', this.logError.bind(this));
-        return serverProcess;
+    protected spawnProcess(command: string, args?: string[], options?: cp.SpawnOptions): RawProcess {
+        const rawProcess = this.processFactory({ command, args, options });


when will this process be deleted from the manager? as I read the code it seems never except onStop

Removing them should be managed by the ProcessManager, since we might want to keep a certain amount of history about closed processes.

We decided to automatically unregister processes when they are killed until we have a different way of cleaning up terminated processes in the ProcessManager.

I don't see the process being killed in the code ?

Also I think it may be good to unregister the process after all automatically but it should be done in a on('exit' callback and registered by the processManager.

This is fine a long as the Process is not disposable. That way even if it's unregistered references to that process still exists and will receive the onDeleted event.

It is killed when the connection is closed:
https://github.com/theia-ide/theia/blob/3ee55262a0d2a2758e680d1d845b81e746e86b48/packages/languages/src/node/language-server-contribution.ts#L71

during forwarding, if one connection get closed another will be closed as well

the on('exit' could also be in the Process class

Ha ok thx my comments still apply

akosyakov · 2017-11-20T19:18:14Z

No it happens when you stop the lsp.
The process has no auto cleanup mecanism

@hexa00 could you show where does it happen in the code?

hexa00 · 2017-11-20T19:56:00Z

@akosyakov It doesn't happen anymore seems I was commenting on oudated code, I just mean that the lsp should call delete if it needs to because the process won't clean itself on exit.

akosyakov · 2017-11-21T07:11:37Z

packages/languages/src/node/language-server-contribution.ts

+    /**
+     * Called on shut down. Only synchronous operations are permitted.
+     */
+    stop?(): void;


this API is misleading

start method is called each time when a new connection is established to start a language server, it can happen multiple times for the same client, e.g. the connection got lost and LanguageClient decided to restart the server.

I would expect that stop means to stop a language server and it should be called each time when the connection got lost. For this, though there is already clientConnectiont.onClose event, which btw can be used to remove a process from the process manager.

I think the following API would be better:

start(clientConnection: IConnection): Disposable;

then a returned disposable can be disposed when the connection got lost as well as onStop.

Thanks for the note, that's a leftover.

hexa00 · 2017-11-21T16:06:44Z

packages/process/src/node/process-manager.ts

+     */
+    unregister(process: Process): void {
+        if (!process.killed) {
+            process.kill();


So if a process is not killed first it will not be unregistred ?
So you have to call it twice ?

I think the return line should be removed.

Why? Can you explain? Thank you!

Well if it's not killed killed is called and returns before calling unregister

I think you are wrong: process.kill() will invoke processManager.unregister(this) after setting killed to true.

Ho yes that is quite weird however imo... I'm commenting in another comment on what I think should be done.

Ho yes that is quite weird however imo...

I am open to any other approach. Since you came up with the initial implementation of the processes and the process manager, maybe you could advise how to unregister the process automatically from the manager on termination. Any ideas, and then I can update the PR with that? Thank you!

Yes see: #841 (comment)
I think this will work fine

hexa00 · 2017-11-21T16:14:00Z

packages/process/src/node/raw-process.ts

@@ -60,6 +62,8 @@ export class RawProcess extends Process {
    kill(signal?: string) {
        if (this.killed === false) {
            this.process.kill(signal);
+            this.processManager.unregister(this);


This is wrong here for a few reasons:

kill may have another signal than SIGTERM here, the user may send SIGHUP for example and thus the process shoud not be unregistred.

We should not unregister the process on SIGTERM to keep history, and if we did for now do that we would need to leave the kill to the unregister call, othewise we call kill twice and that could mean sending a kill signal to the wrong pid.

I think the LSP contribution should clean up it's processes by calling processManager.unregister(process)

That should be the general way to unregister a process. Otherwise we could add a Process->unregister() function that calls the processManager->unregister()

We should not unregister the process on SIGTERM to keep history

That is a very good idea, but this PR has no intention doing anything with the history right now.

I think the LSP contribution should clean up it's processes by calling processManager.unregister(process)

I think it would be odd. So when I create a process via the factory, then the process is registered into the system. Registration is done "automagically" but the unregister code should be invoked explicitly.

Yes I also this it's odd.

I think the best solution would be as described a bit in : #841 (comment)

In Process class register a on('exit') callback and call processManager->unregister()

Kill doesn't call unregister

That way processes will be cleaned-up automatically and terminal references will get notified of that using the delete event from processManager

It will also avoid checking for the signal type etc in kill.

Oops sorry it needs to be the ProcessManager that registers the on('exit') callback otherwise the Process will have a reference to itself and not get cleaned up.

In Process class register a on('exit') callback and call processManager->unregister()

What do you think, shall we remove the on('exit') => unregister from the TerminalBackendContribution? Seems to be a duplicate.

I don't see such a callback ? You mean ws.on('close' ? this one is used to kill the process.

We could change it to kill once the auto unregister is done.

hexa00 · 2017-11-21T16:14:07Z

packages/process/src/node/terminal-process.ts

@@ -71,6 +86,7 @@ export class TerminalProcess extends Process {
    kill(signal?: string) {
        if (this.killed === false) {
            this.terminal.kill(signal);
+            this.processManager.unregister(this);


Not good for the same reasons as RawProcess

kittaakos · 2017-11-21T20:46:33Z

@hexa00, does this look correct? Maybe I am just tired; the process registration and the deletion are done with different IDs. We register with the sequence ID of the process manager then we try to delete it with the ID of the process. I think we have the same problem in this PR too. Please confirm. Thank you!

Get looks OK, if you one pass in the returning number from the register but neither with the process ID nor the PID.

kittaakos · 2017-11-21T21:33:30Z

does this look correct

I think so. 👍 Sorry for the noise.

hexa00 · 2017-11-21T23:14:38Z

Not sure what you're refering to exactly but the Process.id is the same as the ProcessManager id and that's all that is used to handle the processes.

The real process PID is never used.

hexa00 · 2017-11-21T23:15:36Z

@kittaakos I could do a commit in your PR to fix the issues tomorrow ?

kittaakos · 2017-11-27T08:23:45Z

I could do a commit in your PR to fix the issues tomorrow ?

@hexa00, thank you! I will take care of it.

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

Replaced `child_process` with `RawProcess` in LS contribution. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

Instead of terminating all the LS contribution processes, we shut down all processes spawned from the process manager. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

On both `exit` and `error`. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

kittaakos · 2017-11-27T10:56:46Z

I have updated the PR again. From now on, the processes will be unregistered from the manager on both exit and error events. Please review.

CC: @hexa00

hexa00

The term lifecyle is OK now thanks!
Just a few details left.

hexa00 · 2017-11-27T12:12:04Z

packages/process/src/node/process-manager.ts

+     */
+    protected unregister(process: Process): void {
+        const processLabel = this.getProcessLabel(process);
+        this.logger.info(`Unregistering process. ${processLabel}`);


I think info is a big much here ? debug seems more appropriate.
The user will get notified that the process is killed at the info level, however I don't think the user cares about the internal registry.

Same with file watching, right? We use info for file-watching. I disagree with you, but I want this PR to get merged. I will change it to debug as you have requested.

File watching is user visible however ? while this is not... thx for the change.

hexa00 · 2017-11-27T12:12:20Z

packages/process/src/node/process-manager.ts

+        const processLabel = this.getProcessLabel(process);
+        this.logger.info(`Unregistering process. ${processLabel}`);
+        if (!process.killed) {
+            this.logger.info(`Ensuring process termination. ${processLabel}`);


hexa00 · 2017-11-27T12:12:27Z

packages/process/src/node/process-manager.ts

+        }
+        if (this.processes.delete(process.id)) {
+            this.deleteEmitter.fire(process.id);
+            this.logger.info(`The process was successfully unregistered. ${processLabel}`);


hexa00 · 2017-11-27T12:15:08Z

packages/process/src/node/terminal-process.ts

+            }
+        });
+    }
+


This should be removed as we discussed here: (#841 (comment)) stdin is already in the pty we don't have to write it ourselves.

hexa00 · 2017-11-27T12:16:34Z

packages/process/src/node/terminal-process.ts


        this.terminal = pty.spawn(
            options.command,
            options.args,
            options.options);

        this.terminal.on('exit', this.emitOnExit.bind(this));
-        this.output = new TerminalReadableStream(this.terminal);
+        this.output = new ReadableTerminalStream(this.terminal);
+        this.input = new WritableTerminalStream(this.terminal);


Remove this.input, the to write to a term the one should use the write method directly or we could make a stream over write...

Also changed the info log level to debug in the process manager. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

kittaakos · 2017-11-27T14:57:17Z

We have the green builds. I am merging this PR to the master.

kittaakos force-pushed the GH-815 branch from b4a3d07 to 33c7342 Compare November 15, 2017 21:22

hexa00 reviewed Nov 16, 2017

View reviewed changes

kittaakos mentioned this pull request Nov 16, 2017

Enabling general restarting of language servers via integration of LanguageClient with ProcessManager #817

Closed

hexa00 reviewed Nov 16, 2017

View reviewed changes

akosyakov reviewed Nov 16, 2017

View reviewed changes

kittaakos force-pushed the GH-815 branch 2 times, most recently from 5f4a5df to 5219954 Compare November 20, 2017 12:21

kittaakos changed the title ~~[WIP] GH-815: Replace child_process with the Process API in the LS backend contribution~~ GH-815: Replace child_process with the Process API in the LS backend contribution Nov 20, 2017

kittaakos requested a review from svenefftinge November 20, 2017 13:19

akosyakov reviewed Nov 20, 2017

View reviewed changes

akosyakov reviewed Nov 21, 2017

View reviewed changes

kittaakos force-pushed the GH-815 branch from c9451b3 to a1d617a Compare November 21, 2017 08:19

svenefftinge approved these changes Nov 21, 2017

View reviewed changes

akosyakov approved these changes Nov 21, 2017

View reviewed changes

hexa00 suggested changes Nov 21, 2017

View reviewed changes

kittaakos added 8 commits November 27, 2017 11:48

GH-815: Simplified the process API a bit.

b481ee7

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

GH-815: Exposed writable stream process API.

aa32dd8

Replaced `child_process` with `RawProcess` in LS contribution. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

GH-815: Initial changes to support LS process termination.

ab689d0

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

GH-815: Fixed process ID generation. Replaced type with interface.

ad7deea

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

GH-815: Fixed incorrect signal event when terminating process.

e45dfa5

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

GH-815: Replaced process ID array with disposables.

580dad7

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

GH-815: Made process shutdown a bit more generic.

1e060a3

Instead of terminating all the LS contribution processes, we shut down all processes spawned from the process manager. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

GH-815: Made sure process is automatically unregsitered.

9558848

On both `exit` and `error`. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

kittaakos force-pushed the GH-815 branch from d347300 to 9558848 Compare November 27, 2017 10:51

hexa00 suggested changes Nov 27, 2017

View reviewed changes

GH-815: Removed terminal writeable stream.

b5b9e0b

Also changed the info log level to debug in the process manager. Signed-off-by: Akos Kitta <kittaakos@gmail.com>

hexa00 approved these changes Nov 27, 2017

View reviewed changes

kittaakos merged commit 6601c8f into master Nov 27, 2017

kittaakos deleted the GH-815 branch November 27, 2017 14:57

GH-815: Replace child_process with the Process API in the LS backend contribution #841

GH-815: Replace child_process with the Process API in the LS backend contribution #841

Conversation

kittaakos commented Nov 15, 2017 • edited Loading

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

hexa00 Nov 16, 2017 • edited Loading

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

hexa00 commented Nov 16, 2017

hexa00 commented Nov 16, 2017

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 commented Nov 16, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexa00 commented Nov 16, 2017 • edited Loading

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 commented Nov 20, 2017

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

Choose a reason for hiding this comment

akosyakov Nov 21, 2017 • 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 Nov 20, 2017

hexa00 commented Nov 20, 2017

akosyakov Nov 21, 2017 • edited Loading

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

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 commented Nov 21, 2017

kittaakos commented Nov 21, 2017

hexa00 commented Nov 21, 2017

hexa00 commented Nov 21, 2017

kittaakos commented Nov 27, 2017

kittaakos commented Nov 27, 2017 • edited Loading

hexa00 left a comment

GH-815: Replace `child_process` with the `Process` API in the LS backend contribution #841

GH-815: Replace `child_process` with the `Process` API in the LS backend contribution #841

kittaakos commented Nov 15, 2017 •

edited

Loading

hexa00 Nov 16, 2017 •

edited

Loading

kittaakos commented Nov 16, 2017 •

edited

Loading

hexa00 commented Nov 16, 2017 •

edited

Loading

akosyakov Nov 21, 2017 •

edited

Loading

akosyakov Nov 21, 2017 •

edited

Loading

kittaakos commented Nov 27, 2017 •

edited

Loading