Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make process creation asynchronous, improve error reporting #3447

Closed
wants to merge 1 commit into from

Conversation

simark
Copy link
Contributor

@simark simark commented Nov 8, 2018

The error reporting when spawning a process fails (notably ENOENT and
EACCES) is not very good currently.

For example, after trying to execute a non-executable file, we get a
RawProcess object with the process property undefined. When we try to
execute a non-existent file, we get a RawProcess.process with an
undefined pid. I think it's not very practical to be able to receive
RawProcess objects that represent processes that we failed to start.

With this patch, I propose to extract the actual process spawn from the
RawProcess and TerminalProcess constructors and move that in some
factory methods that return Promises. The RawProcess and
TerminalProcess objects will be created to wrap successfully started
processes. Errors will be propagated by returning rejected Promises.

Doing this change has quite a bit of impact on other modules, but I
think that in the end it's an API that's harder to misuse.

Change-Id: I76bc0cc5ab0664c04ddc73e4d68e78948780f9d0
Signed-off-by: Simon Marchi simon.marchi@ericsson.com

The error reporting when spawning a process fails (notably ENOENT and
EACCES) is not very good currently.

For example, after trying to execute a non-executable file, we get a
RawProcess object with the `process` property undefined.  When we try to
execute a non-existent file, we get a RawProcess.process with an
undefined pid.  I think it's not very practical to be able to receive
RawProcess objects that represent processes that we failed to start.

With this patch, I propose to extract the actual process spawn from the
RawProcess and TerminalProcess constructors and move that in some
factory methods that return Promises.  The RawProcess and
TerminalProcess objects will be created to wrap successfully started
processes.  Errors will be propagated by returning rejected Promises.

Doing this change has quite a bit of impact on other modules, but I
think that in the end it's an API that's harder to misuse.

Change-Id: I76bc0cc5ab0664c04ddc73e4d68e78948780f9d0
Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
@simark
Copy link
Contributor Author

simark commented Nov 8, 2018

The main driver for this patch is actually that I want to get rid of this findCommand here. But we need to precisely be able to know when a task has failed to start or has started with success.

https://github.com/theia-ide/theia/blob/d572d5f34cea78b37653b117c5d6bc8c5e23ba79/packages/task/src/node/process/process-task-runner.ts#L91-L98

@akosyakov
Copy link
Member

akosyakov commented Nov 9, 2018

It looks like a breaking change for all external extensions contributing language servers.

//cc @svenefftinge

@akosyakov
Copy link
Member

akosyakov commented Nov 9, 2018

@simark Can you elaborate why is it difficult to know? As I can see RawProcess api mimic node.js api, so you should be able to create an instance and immediately install on error listener to get notified if it is failed.

As an alternative, we could create a utility on the top of existing APIs to promisify the process creation without breaking everything.

@simark
Copy link
Contributor Author

simark commented Nov 9, 2018

@simark Can you elaborate why is it difficult to know? As I can see RawProcess api mimic node.js api, so you should be able to create an instance and immediately install on error listener to get notified if it is failed.

A concrete example is the requirement for tasks. We need to know at some point if the process has been started properly, so we can show a "Task X started" message. Otherwise, we need to know if there has been an error (as well as what is the error) so we can show a "Task X failed to start (reason XYZ)" message. It's easy to know when there is an error - you just listen for the event. But how do you know if/when the process has been successfully started?

As an alternative, we could create a utility on the top of existing APIs to promisify the process creation without breaking everything.

Perhaps.

@simark
Copy link
Contributor Author

simark commented Nov 9, 2018

I'm thinking we could add a 'started' event to RawProcess, to indicate the process has been successfully started. The idea would be that you are guaranteed to have either your started or error callback called. You would need to wait for that started event before doing anything with your process. It wouldn't break the API per-se, but all current usages would remain wrong. For example:

const rawProcess = createProcess('/bin/non-executable-file');
rawProcess.process.output; // rawProcess.process is undefined

I wonder which one is better:

  • break the API, which will cause compilation failure and force people to look into it and fix their code (which would be harder to get wrong hopefully, if we have a Promise-based API)
  • don't break the API by adding a new event, but people's code will remain buggy

}

get pid() {
return this.process.pid;
}

get stdin() {
Copy link
Contributor Author

@simark simark Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have renamed input to stdin, output to stdout, errorOutput to stderr. It is not a necessary change, so I can revert that to reduce the number of breaking changes.

@akosyakov
Copy link
Member

akosyakov commented Nov 9, 2018

but people's code will remain buggy

Which code? The whole code in repo does not check pid and works perfectly.

For the task extension, you can check pid, if it is not undefined consider a process started.

@simark
Copy link
Contributor Author

simark commented Nov 9, 2018

Which code? The whole code in repo does not check pid and works perfectly.

It works, but I wouldn't say it works perfectly. It doesn't handle EACCES problems correctly.

For the task extension, you can check pid, if it is not undefined consider a process started.

That makes it a pretty bad API, that's easy to mis-use. I would like if we made the situation better so that it's not possible for anybody else to fall in the same trap in the future.

The pid === undefined thing is not even documented in node's child_process module, it's what I found by experimenting. That's why I'd like to encapsulate as much as possible of node's inconsistent API in something more robust.

@svenefftinge
Copy link
Contributor

svenefftinge commented Nov 13, 2018

That is indeed a massively breaking change.
In this comment someone from node says that there is no way to tell if a process was successfully spawned other than using it : nodejs/help#1191 (comment)

We could document the pid === undefined case in our API. Wouldn't that help already? So clients could do what you tried to do generically.

@simark
Copy link
Contributor Author

simark commented Nov 13, 2018

That is indeed a massively breaking change.
In this comment someone from node says that there is no way to tell if a process was successfully spawned other than using it : nodejs/help#1191 (comment)

Yes, I agree. It depends what we mean by "successfully spawn" the process. If you pass wrong flags to your process and it exits right away, I consider that it has been successfully spawn (there has been a process spawned, which has then exited). Even in the case of missing shared library (at least on Linux), the process is successfully spawned, it just happens to fail during execution of userspace code. But there was a process spawn, it had a valid pid and everything, it just exited rather quickly.

We could document the pid === undefined case in our API. Wouldn't that help already? So clients could do what you tried to do generically.

That would be enough for the ENOENT case, but not the EACCES case. In the EACCES case, the process property is actually undefined, even though the typing says it can't be undefined:

https://github.com/theia-ide/theia/blob/master/packages/process/src/node/raw-process.ts#L54

so we can't even check for pid === undefined, because that would dereference the undefined process. To align the API with the actual behavior, we would need to make that property:

readonly process: ChildProcess | undefined;

but even that would be an API break.

To fix my use case (the tasks) with the minimal, I am thinking of implemented what I suggested earlier, which is to add a 'started' event. This event would be emitted by the constructor of RawProcess if process.pid !== undefined (inside a process.nextTick to let users the change to register a callback). Receiving that event would be the confirmation that the process has been successfully started (using the above definition of "successfully").

@akosyakov
Copy link
Member

akosyakov commented Nov 15, 2018

@simark Could you open a PR without breaking changes?

@simark
Copy link
Contributor Author

simark commented Nov 15, 2018

@simark Could you open a PR without breaking changes?

Yes, eventually.

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Jan 31, 2020

@simark I'm closing the pull-request for the moment because:

I understand you are quite busy but if you ever get the chance to find the time to re-work the pull-request feel free to re-open it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process issues related to the process extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants