[READY] Properly shut down on Windows

[micbou]

Problem

When killing a process on Windows, it is terminated like it received a SIGKILL signal on Linux. This means that @atexit functions will not be called. In our case, ServerCleanup is not executed:
completers Shutdown methods are not called, neither the Shutdown function in the global extra conf. In particular, subservers like OmniSharpServer and Tern.js are left running.
This issue happens in two situations:

• a client like YCM terminates the ycmd server;
• watchdog plugin pulls the trigger.

How to solve it?

Solution

First situation should be solved by adding a new handler /shutdown to properly shut down the ycmd server from a client. Unfortunately, waitress and bottle don't provide an easy way to shut it down. Using sys.exit() in the /shutdown handler will not work because it is run in a thread. The solution is to use the same mechanism as in the watchdog plugin. After cleaning up, we start a thread to immediately kill the server.
Second situation is fixed by using the function associated to the /shutdown handler in the watchdog plugin.

Tests

To test that the shutdown handler and the watchdog plugin are properly working, we need to start the ycmd server for real. We cannot use webtest here. I added 4 tests, which are a combination of shutting down ycmd without and with a subserver, from a request and from the watchdog plugin.
psutil module was added to the test requirements to work with processes in a portable way.
In order to test the watchdog plugin, I added a command-line argument to set the check_interval_seconds variable.
Since we are starting subprocesses in those tests, some changes were needed to enable coverage in Python subprocesses. Another change to improve coverage was to update Waitress to version 0.8.10. See the commit message for some explanation.
A benefit of those tests is a significant increase in coverage (~6%).

Caveats

• Tests may introduce flakyness. Robustness should be improved;
• Coverage seems to cause issue with the Gocode daemon. Not sure if it is related to this PR. Need to investigate.

Next

• Update YCM to use the shutdown handler (fix issue Omnisharp server is not stopped on Gvim exit YouCompleteMe#876);
• Fix logfiles clean up on Windows.

[Valloric]

This is important work; thanks for taking it on! I didn't know we left sub-servers running after ycmd exits on Windows. That's a pretty bad bug.

[vheon]

This PR is really awesome and needed! 👍 Is also pretty straightforward to understand since @micbou explained well what was needed. Anyhow I left a comment on a file since I don't understand why the sleep is needed, and if is needed how do we know that 0.1 is enough? I generally don't like sleeps. The only file that is left for me to review is the server_test; the reason is that I see in that file lots of function which are similar or used also in the example_client or in ycmd itself, such the HMAC signing and checking, the passing of the options through a temporary file, some utf8 stuff like _RecursiveEncodeUnicodeToUtf8 and some other things. So to the question: should we refactor this right now? Or should we do a second pass to do more analysis and refactor later?

---

Reviewed 9 of 10 files at r1.
Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Why is this sleep needed?

---

Comments from the review on Reviewable.io

[micbou]

AppVeyor failures are not related to this PR. See PR #283.

So to the question: should we refactor this right now? Or should we do a second pass to do more analysis and refactor later?

I think this is better to refactor now than later. I already did some refactoring by reusing functions from the hmac_utils module. I just found out that _ToUtf8Json and _RecursiveEncodeUnicodeToUtf8 are already defined in the utils module so I will use them. Tell me if there are other functions.

---

Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Good question. It is needed to let the time for the server to send the response before being killed by the thread. I should add a comment to explain this.

---

Comments from the review on Reviewable.io

[vheon]

Before this will land into master we should ping @abingham (emacs-ycmd), @Qusic (atom-youcompleteme), @mawww (kak-ycmd) since this will change how to shutdown the ycmd server.

---

Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from the review on Reviewable.io

[puremourning]

Another thing to consider refactoring is the class which performs full integration tests. A large part of the class in server_test.py is just boilerplate for setting up and tearing down the server. Maybe this could be a base to be implemented by something actually containing the _test() methods. That sort of separates infrastructure from actual tests.

Would potentially also allow more separation, such as testing Tern completer's "shutdown" method within the tests/javascript directory and other Shutdown methods for the respective completers.

That might also give us a canonical minimal ycmd client, which could be useful or educational in its own right.

---

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

examples/example_client.py, line 110 [r1] (raw file):
I know this is only an example, but we should probably clean up the server PID handle here. We're expecting the postcondition of this method to be that the server is no longer running. Is there a way to block until the pid exits? Like thread.join() or equivalent? Maybe with a more forcefull kill if it times out? May be scope creep for this example client, but perhaps at least

if self.IsAlive():
  # the server didn't terminate for some reason. kill it.
  self._popen_handle.terminate()

it looks like psutil has a method for waiting: http://pythonhosted.org/psutil/#psutil.Process.wait
or maybe http://pythonhosted.org/psutil/#psutil.wait_procs

---

travis/travis_install.sh, line 39 [r1] (raw file):
can we break this into 2 lines?

echo -e "..." > \
  ${YCMD_VENV_DIR}/....

I think that works...

---

ycmd/main.py, line 94 [r1] (raw file):
The difference between --idle_suicide_seconds and --check_interval_seconds seems quite subtle. The help and comments don't really make it obvious which one I care about in which situation.

---

ycmd/main.py, line 155 [r1] (raw file):
"These" is the plural form of "This", "Those" is the plural form of "That". Pedantry, I know :)

---

ycmd/handlers.py, line 219 [r1] (raw file):
We should add a comment here that while this method is a handler for /shutdown it is also called by the watchdog plugin which will not have a response context to write to (nor a request to read from). Either that, or we should make it explicit by having this "handler" call a non-handler method. This might be better in fact - it feels... wrong somehow (to me) to call a request handler from anywhere but bottle.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Presumably, the pause is so that the /shutdown command has time to return HTTP 200 to the client before actually shutting down the server. I'm not a fan of this arbitrary delay. As there is no synchronisation, it is still possible for this tread to execute prior to the response being sent.

I feel we should either try to synchronise it (I'm not sure how), or just exit synchronously and state in the API that no response is received to a shutdown request and the communication session is terminated (TCP reset). I'm guessing that you probably tried this and had some sort of problem with the client and/or tests?

---

ycmd/tests/server_test.py, line 180 [r1] (raw file):
I think this proc scans every process in the system. This might be quite slow.

Could we use psutil's Process class? It seems to have a children method:

children(recursive=False)[source]
Return the children of this process as a list of Process objects, pre-emptively checking whether PID has been reused. If recursive is True return all the parent descendants. Example assuming A == this process:

---

ycmd/watchdog_plugin.py, line 45 [r1] (raw file):
Hmm. I see both of these options existed before. I still can't claim to understand what they do, or how we use them in the test. Probably just me being dumb.

Do we document the ycmd options anywhere else, like in the README.md ?

---

Comments from the review on Reviewable.io

[vheon]

Review status: 5 of 10 files reviewed at latest revision, 8 unresolved discussions.

---

ycmd/watchdog_plugin.py, line 45 [r1] (raw file):
The Watchdog plugin check every check_interval_seconds how much time is passed since the last request the ycmd server have served. If this time is over the idle_suicide_seconds then the Watchdog plugin will kill the ycmd server.

The description is in the watchdog plugin https://github.com/Valloric/ycmd/blob/8d6cf88518a4cbd90d242abbf13d25b39fee1552/ycmd/watchdog_plugin.py#L29

---

Comments from the review on Reviewable.io

[puremourning]

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from the review on Reviewable.io

[micbou]

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

examples/example_client.py, line 110 [r1] (raw file):
We can use self._popen_handle.wait() but there is no timeout so it may block the script indefinitely.

---

travis/travis_install.sh, line 39 [r1] (raw file):
Done.

---

ycmd/main.py, line 155 [r1] (raw file):
Done.

---

ycmd/handlers.py, line 219 [r1] (raw file):
I chose the "handler calls a non-handler" path. I added specific logging for the watchdog shutdown to differentiate it from the shutdown one.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Using Bottle after_request hook could be the solution. I'll test it.

---

ycmd/tests/server_test.py, line 180 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[micbou]

ycmd/handlers.py, line 219 [r1] (raw file):
I mean the "handler one".

---

Comments from the review on Reviewable.io

[vheon]

Reviewed 4 of 5 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Great!

---

ycmd/tests/server_test.py, line 93 [r2] (raw file):
can we wrap this in a function like StartCompleterServer?

---

Comments from the review on Reviewable.io

[vheon]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/main.py, line 94 [r1] (raw file):
Which "I" are we talking about? Are we talking about "I" the user of ycmd, the "I" user of YCM or "I" developing ycmd??

---

Comments from the review on Reviewable.io

[puremourning]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/main.py, line 94 [r1] (raw file):
Any/all of the above (except, perhaps the user of YCM). It's fine - I think your explanation below is clear.

---

Comments from the review on Reviewable.io

[mawww]

Does the SIGTERM signal still works correctly ? or do we need to go through the shutdown command now ?

kak-ycmd is not expected to run directly on windows, but always on cygwin, so I assume SIGTERM will still work there, which means its it not urgent to tweak kak-ycmd to support the shutdown command if SIGTERM is still a supported way to close ycmd.

[abingham]

Thanks for looping me in on this. It sounds like the only substantial change from the client point of view is that we'll call /shutdown rather than killing the process. If that's the case, this should be a very simple transition for emacs-ycmd. (And if not, please enlighten me!)

[micbou]

Another thing to consider refactoring is the class which performs full integration tests. A large part of the class in server_test.py is just boilerplate for setting up and tearing down the server. Maybe this could be a base to be implemented by something actually containing the _test() methods. That sort of separates infrastructure from actual tests.

I moved the infrastructure to a new class Client_test. Tests are now in Shutdown_test, subclass of Client_test.

Does the SIGTERM signal still works correctly ? or do we need to go through the shutdown command now ?

In PR current state, you can still use the SIGTERM signal to stop the server and it should stay that way. Our goal is to provide a portable (and therefore recommended) way to shutdown the server.

---

Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Unfortunately, this doesn't work. So, I see three ways to move forward:

• keep it that way: kill the server from a thread started in the handler function after a fixed delay. Advantages: easy to implement and a 200 response should be returned. Inconvenient: client cannot completely rely on the 200 response because server may be killed before sending the response;
• directly kill the server in the handler function. Advantages: easy to implement. Inconvenient: no response (connection aborted);
• implement a subclass of TcpWSGIServer from waitress and add a shutdown method like the one in StopableWSGIServer from webtest. Advantages: a 200 response should be returned (need to implement it to be sure). Inconvenient: not straighforward to implement, depend on waitress internals.

---

Comments from the review on Reviewable.io

[puremourning]

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/handlers.py, line 269 [r1] (raw file):
My gut tells me the best thing to do is the second option, as it is the safest and simplest. If it is a pain for clients, how much of a pain, taking, say YCM client as example?

---

Comments from the review on Reviewable.io

[Valloric]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Agreed with @puremourning, second option seems the most sensible. I think it's perfectly fine for a /shutdown handler to not return anything.

---

Comments from the review on Reviewable.io

[micbou]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/handlers.py, line 269 [r1] (raw file):
I tried third approach and it looks good. It does the job better than other solutions by reaching the end of the program and not killing the process. In particular, @atexit function is called on Windows. It is also relatively simple. I'll push the changes this week-end and we'll discuss them.

If it is a pain for clients, how much of a pain, taking, say YCM client as example?

Just a matter of a try-except block. Even if the server returns a 200 response, such block is needed for timeouts.

---

Comments from the review on Reviewable.io

[vheon]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/handlers.py, line 269 [r1] (raw file):
I'm actually worried about the cons part of the third approach

Inconvenient: not straighforward to implement, depend on waitress internals.

---

Comments from the review on Reviewable.io

[Valloric]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/handlers.py, line 269 [r1] (raw file):
Depending on waitress internals is a deal breaker though; we only want to rely on the WSGI interface so that we can swap out waitress for something else if necessary. This isn't merely a theoretical concern; when I was writing ycmd, initially it used a different server (can't remember which) and I switched it to waitress for performance reasons days before it went live. The switch was something like 3 lines of code and that's it.

If we were ever decide to lock ourselves into a particular server framework, it's going to be for a very good reason. This case doesn't strike me as that. Not returning from a /shutdown handler isn't pretty, but it's not a big deal.

---

Comments from the review on Reviewable.io

[homu]

☔ The latest upstream changes (presumably #296) made this pull request unmergeable. Please resolve the merge conflicts.

[vheon]

@micbou is something still missing? Can we call homu on this?

[micbou]

Rebased and squashed. This is ready now.

---

Reviewed 1 of 1 files at r19, 1 of 1 files at r20.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[Valloric]

Awesome, let's go!

@homu r=puremourning

[homu]

📌 Commit ed23b23 has been approved by puremourning

[homu]

⚡ Test exempted - status

[ptrv]

The server startup behavior changed also a bit, didn't it? After start there is no print serving on http://127.0.0.1:<port> anymore. Emacs ycmd used to parse the port number from there. Since we are not specifying the port number in Emacs ycmd. We used the automatically assigned one from the server.

[puremourning]

@ptrv does that mean this commit breaks emacs-ycmd ?

How is the ycmd version tied to emacs-ycmd? I seem to recall that some folk install it independently? If so then I think we may have a problem that we need to fix.

[ptrv]

Yes this commit breaks emacs-ycmd. Unfortunately we do not tie a specific ycmd version to it. Probably we should, but that's different story.

The quick fix in emacs-ycmd would be to specify the port for the server on emacs-ycmd side.

[puremourning]

The difference is because we are no longer calling waitress.serve rather doing that job ourselves. We can add an equivalent print.

[ptrv]

That would help a lot for now. and then emacs-ycmd needs to implement a way to tie the server version.

[puremourning]

@ptrv any chance you could kick the tyres on the latest commit ? I don't have an emacs at hand...

[ptrv]

@puremourning The patch works. Thanks!

[puremourning]

Cool. I did try and make an emacs that worked with ycmd, but i'm too much of an emacs noob to make it work. i tried spacemacs and, well, none of the ycmd- commands seems to do anything. So thanks for checking, i'll stick to vim ^_^