PR: Kill LSP transport layer if Spyder gets killed

[andfoy]

Description of Changes

This PR adds a periodical check for the parent process of the LSP transport layers. This PR, alongside palantir/python-language-server#642 will prevent the existence of zombie process if Spyder crashes unexpectedly

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:
@andfoy

[ccordoba12]

I have two questions for you:

1. if our transport client is killed, then the PyLS also is also killed?
2. If 1. is true, then shouldn't we start the PyLS with the check_parent_process enabled? (I think that's not the case right now).

[andfoy]

if our transport client is killed, then the PyLS also is also killed?

Right now, we will kill it only if It is stdio. When the equivalent PR on pyls gets merged, then It Will also kill TCP servers

[andfoy]

The mechanism works like a cascade, Spyder kills the transport process, which in turn, kills pyls.

[ccordoba12]

@andfoy, I left a small review for you, otherwise looks good.

[ccordoba12]

@dalthviz, please try this PR on Windows by forcefully killing Spyder and verifying that both our transport layer and the pyls script are killed automatically after that.

[goanpeca]

Will check on OSX

[dalthviz]

@ccordoba12 it works for me, after killing Spyder with the task manager or Ctrl+C from cmd I don't see any left out Python process

[goanpeca]

On OSX ps aux | grep transport/main.py is empty, so ok

but

ps aux | grep pyls displays

gpena-castellanos 79131   0.0  0.6  4644408 107456 s001  S    12:03PM   0:03.03 /Users/gpena-castellanos/anaconda2/envs/spyder/bin/python -m pyls --host 127.0.0.1 --port 2087 --tcp

Do I need to use the dev version of PYLS @andfoy ?

[andfoy]

but ps aux | grep pyls displays

This depends on palantir/python-language-server#642

[goanpeca]

This depends on palantir/python-language-server#642

Ok, then this is good to go :-)

Needs a new release of PyLS

[ccordoba12]

Sorry guys, I forgot this depends on a new release of the PyLS. I'm going to do one right now.

[andfoy]

@ccordoba12 This requires the latest release of pyls on our Anaconda channel

[ccordoba12]

Yes, sorry about that. I just moved 0.28.3 to our channel.

Please do a git commit --amend and a git push force after that to retrigger our CIs.

[ccordoba12]

@goanpeca, @dalthviz, now you can help us to test this.

First you need to update the PyLS to its 0.28.3 version with

conda install -c spyder-ide python-language-server=0.28.3

Then you need to forcefully kill Spyder and see if both processes mentioned above disappear after ~5 secs.

[goanpeca]

Ok @ccordoba12 will check now

[goanpeca]

@ccordoba12 I am seeing

$ ps aux | grep pyls
gpena-castellanos  1728   0.0  0.6  4650308 108640 s001  S     1:24PM   0:05.75 /Users/gpena-castellanos/anaconda2/envs/spyder/bin/python -m pyls --host 127.0.0.1 --port 2087 --tcp --check-parent-process

[goanpeca]

We need to check this on windows

[ccordoba12]

@andfoy, please merge this one with master to fix the merge conflict.

[CAM-Gerlach]

@ccordoba12 @goanpeca You need me to help test on Windows? I have a major deadline on a project tomorrow morning but I can do it after that if you haven't already.

[ccordoba12]

You need me to help test on Windows?

Sure, that would be really appreciated! But first we need to solve the problems we have at the moment with the PyLS (see palantir/python-language-server#682).

I'll let you know when that's sorted out.

[CAM-Gerlach]

Sure, I'll await your instructions; thanks for letting me know. Hope you can get the Jedi issue resolved soon.

[ccordoba12]

@CAM-Gerlach, this is ready for you to test it. For that you need to update to python-language-server version 0.29.2, which is already present in our channel.

[CAM-Gerlach]

@ccordoba12 Killing the Spyder main process from process explorer does now result in the transport client getting killed immediately and automatically, but the pyls server, the jedi subprocess it launches and the two conhost.exes attached to the same don't ever get killed (I waited 5 minutes and they were still there). Meanwhile, killing it with Ctrl-C kills everything instantly, as AFAIK it did before this patch.

Also, I would like to test what happens if there is first a problem with the client or the server that triggers the crash in the first place; most of the crashes and hangs I experience regularly seem to be associated with PyLS or LSP has stopped working first. For example, just had two hangs and a crash today, and they either shortly or immediately followed LSP stopping responding. I'll keep running under this build and see if I get one of said crashes, though they never show up when I want them to.

[ccordoba12]

@CAM-Gerlach, I think I fixed the errors on Windows in palantir/python-language-server#684

I uploaded a new PyLS version to our channel (0.29.3), so please update to it and let us know if it fixes the problem for you.

[CAM-Gerlach]

@ccordoba12 Yep, updated and it works great! Also, I tested completion, help etc. just to be sure and that all works too. Thanks!

[ccordoba12]

Great, thanks for the confirmation!

[ccordoba12]

Thanks a lot @andfoy for this!