Server.kill: use :quitall

[blueyed]

The previous method of closing input/output unconditionally will cause
Vim to end with SIGHUP, which causes a wrapping process using Python's
subprocess module to abort:

10342      0.000166 write(1, "Vim: Finished.\r\n", 16) = -1 EIO (Input/output error)
10342      0.000169 rt_sigaction(SIGHUP, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7ff4714958f0}, {sa_handler=SIG_IGN, sa_mask=[HUP], sa_flags=SA_RESTORER|SA_REST>
10342      0.000122 rt_sigprocmask(SIG_UNBLOCK, [HUP], [HUP], 8) = 0
10342      0.000081 getpid()            = 10342
10342      0.000058 kill(10342, SIGHUP) = 0
10342      0.000103 --- SIGHUP {si_signo=SIGHUP, si_code=SI_USER, si_pid=10342, si_uid=1000} ---

Using :quitall! and waiting for Vim to finish is cleaner.

This also uses SIGTERM instead of SIGKILL.

Fixes #51.

[AndrewRadev]

Killing the server with TERM instead of KILL makes a lot of sense, for sure. If nothing else, we could do the "TERM -> wait for timeout -> KILL after 3 seconds" thing you're doing. Quitting with :quitall is a viable strategy as well, but I'd rather have it a separate method. Maybe we could have a def quitall! and a separate def kill? If nothing else, doing a kill -9 is a super fast way to get rid of the server in the tests :). With the :quitall, on my machine, tests are about 14s, from 8s with kill -9.

What do you think? Would it be good enough to have two methods? A quitall! that sends the command and then waits with timeouts, and a kill that does a TERM followed by a KILL?

[AndrewRadev]

Also, I have no idea what order the @r.close; @w.close have to be done in, whether before or after. Could you explain why you've moved them to the end? Does that solve some issue?

[blueyed]

Your plan sounds good.

Would it be good enough to have two methods?

Sure.

I've moved @r.close; @w.close since closing input/output would send SIGHUP already.

But I was a bit confused myself, given that I run Vim wrapped in covimerage (a Python program that uses subprocess, and e.g. SIGHUP was not forwarded to Vim), and then also by having Vim segfault itself in certain situations.

With the :quitall, on my machine, tests are about 14s, from 8s with kill -9.

Is kill -9 faster than kill -TERM also?

Depending on what you used the sleep 0.1 while Process.waitpid(@pid, Process::WNOHANG).nil? might add some overhead already timewise.

Not sure what to do here - maybe using TERM would be a good first step (waiting up to X seconds and then using KILL), without the quitall?

[AndrewRadev]

Is kill -9 faster than kill -TERM also?

Depending on what you used the sleep 0.1 while Process.waitpid(@pid, Process::WNOHANG).nil? might add some overhead already timewise.

Not sure what to do here - maybe using TERM would be a good first step (waiting up to X seconds and then using KILL), without the quitall?

First off, it's not a huge deal. The Vimrunner test suite itself probably calls kill a lot more often than most Vim plugin test suites, if they set reuse_server = true in their helpers. A single Vim instance that's cleared after every test would usually work just fine.

The slowdown is probably due to whatever cleanup a Vim instance does when it's closed, maybe cleaning temporary files, maybe some other work. One extra command probably adds to it as well. With a TERM kill, the test suite is 11s, which is literally right in the middle. The waitpid is probably not a problem, since it never really gets all the way to the timeout. It might add an extra 100ms or so because of the sleep 0.1, but that's probably fine.

What I would go with is three functions:

• .quitall! that runs that command, waitpids, and then hard-kills if there's a timeout
• .kill (the default) that does a TERM-kill, waitpids, and then hard-kills if there's a timeout
• .hard_kill that does a kill -9, unconditionally

That way, if you really need fast relaunching of a Vim instance, you could hard-kill it, and if you want to be sure everything is cleaned up as in your case, you could use quitall. For most people, I imagine the compromise would be fine?

If you're okay with implementing that, I can then look to expose some configuration option for the test helpers, apart from start_vim, maybe a method called stop_vim, to make testing easier.