electron: fix killing backend

[paul-marechal]

Gracefully kill the backend process when exiting an Electron app. A
check is made to make sure we only kill the backend if it is running.
The risk else is that we could kill a random process with the same PID
as the backend had before exiting. This should have been rare but not
impossible.

Signed-off-by: Paul paul.marechal@ericsson.com

Closes #8804
Closes #8534

How to test

• Run the electron example.
• Kill the backend process.
• Close all Electron windows.
• No error should show.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[kittaakos]

I think ignoring the error is enough. We do it for the plug-ins too:

theia/packages/plugin-ext/src/hosted/node/hosted-plugin-process.ts

Lines 138 to 140 in 0a72196

	if (e && 'code' in e && e.code === 'ESRCH') {
	return;
	}

[kittaakos]

Please swallow the error with code ESRCH. I am not sure if an error on the child_process always means that the process does not exist. An error can also be emitted when, for example, the process termination has failed.

From the docs:

The process could not be killed, or

Please change. Thank you!

[paul-marechal]

My change currently checks that the process is running before trying to kill it (avoiding the ESRCH when the process was already dead). If the process cannot be killed despite that I think an error is ok?

[kittaakos]

I do not want to accept this PR as is because I am not convinced, it is safe. There could be false negative checks. For example, there was an error, but the backend process is still up and running. (Here it is in your code.) In such cases, we do not terminate the backend process on the app quit although we should.

There are applications out there building on top of Theia's behavior, and I am not positive that the proposed change covers all cases. Why do not we just swallow the ESRCH. What's wrong with that?

[paul-marechal]

I wanted to avoid ESRCH completely to avoid the following: child_process.kill

Sending a signal to a child process that has already exited is not an error but may have unforeseen consequences. Specifically, if the process identifier (PID) has been reassigned to another process, the signal will be delivered to that process instead which can have unexpected results.

The implication is that if we allow killing when we somewhat expect to get ESRCH it means we might also kill another random process while doing so.

I added the check as requested.

[kittaakos]

The implication is that if we allow killing when we somewhat expect to get ESRCH it means we might also kill another random process while doing so.

Good catch, maybe we have to change it for other places in Theia.

I added the check as requested.

Much appreciated. Why do not we do just this and nothing more? (I think, you forgot to rethrow the error, so you swallow everything. This is one of the main reasons I try to go for minimum changeset.)

try {
    // If we forked the process for the clusters, we need to manually terminate it.
    // See: https://github.com/eclipse-theia/theia/issues/835
    process.kill(backendProcess.pid);
} catch (error) {
    // If the process wasn't already running then do nothing.
    // See https://man7.org/linux/man-pages/man2/kill.2.html#ERRORS
    if (error.code === 'ESRCH') {
        return;
    }
    throw error;
}

[paul-marechal]

Good catch I was missing the re-throw. I'd keep the rest to address what I talked about earlier.

[paul-marechal]

Why do not we do just this and nothing more?

Because I'm focusing on the Electron backend case. But I agree that it might be something to apply elsewhere too.

This could be a follow up.

[kittaakos]

I'd keep the rest to address what I talked about earlier.

OK. Thanks for the update. 👍 I verify it soon.

Why do not we do just this and nothing more?

Because I'm focusing on the Electron backend case. But I agree that it might be something to apply elsewhere too.

This could be a follow up.

Can you please explain?

do just this and nothing more?

For clarification: as you might have noticed, I am trying to push for minimal changesets and avoid unnecessary changes. Currently, there is no way to verify any proposed change in a bundled electron app built from the sources as part of the review process. There are applications out there using Theia as a framework, and I want to ensure consistency in the core behavior for them.

[paul-marechal]

I don't think anything is unnecessary in the current changes. If we know the backend died, there's no use issuing a kill signal: this is what I minimally implemented.

Regarding the follow up I mentioned, I thought you said that if I address the issue of trying to issue a kill when the process is not running, then we should do it in other places where we kill processes. I answered that this change only focuses on this specific location, and other places can be looked into in a different PR. I might have misunderstood your question, I now see what you meant.

[paul-marechal]

@kittaakos I managed to trim the change to be more minimal.

[kittaakos]

It works. Thank you!