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

fix(Launcher): killing a process can throw #2102

Merged
merged 3 commits into from
Mar 16, 2018
Merged

fix(Launcher): killing a process can throw #2102

merged 3 commits into from
Mar 16, 2018

Conversation

thorn0
Copy link
Contributor

@thorn0 thorn0 commented Feb 26, 2018

... if the target pid does not exist.

... if the target pid does not exist.
@aslushnikov
Copy link
Contributor

@thorn0 can you please make npm run lint happy? I think you're missing space after catch.

@thorn0
Copy link
Contributor Author

thorn0 commented Feb 27, 2018

@aslushnikov done

@aslushnikov
Copy link
Contributor

I was initially positive about this change, but now I'm scared we might be killing some other process. Looks like we're covering the actual problem instead of solving it properly.

@JoelEinbinder, what do you think about this patch?

@JoelEinbinder
Copy link
Contributor

I was initially positive about this change, but now I'm scared we might be killing some other process. Looks like we're covering the actual problem instead of solving it properly.

That seems like a valid concern. Do we get a disconnected event synchronously on our websocket? Maybe we can use that to avoid killing someone else's process with the same id. It is weird that we don't get a 'close' event on the process in time.

@thorn0
Copy link
Contributor Author

thorn0 commented Mar 7, 2018

At least on Windows, the exception occurs only when there is no process with the specified PID. That's the situation the proposed change fixes. I easily reproduce it killing Chromium manually.

If, as you're afraid, it kills some other process with the same reused PID, it won't throw. So I don't see how this change "covers the actual problem".

piscisaureus pushed a commit to propelml/propel_deps that referenced this pull request Mar 14, 2018
@aslushnikov
Copy link
Contributor

At least on Windows, the exception occurs only when there is no process with the specified PID. That's the situation the proposed change fixes. I easily reproduce it killing Chromium manually.

If, as you're afraid, it kills some other process with the same reused PID, it won't throw. So I don't see how this change "covers the actual problem".

I'm afraid we might be killing some other process, if the pid of the chrome process gets re-used after its manual termination. However, since this is highly unlikely and there's no easy way around, I'll merge this in.

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

Successfully merging this pull request may close these issues.

3 participants