Skip to content

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