1369711 - [mozlint] Investigate and fix ability to Ctrl-C lint run

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect, P1)

Description

[Andrew Halberstadt [:ahal]]

Mozlint used to exit gracefully on a KeyboardInterrupt, but now it seems that the process hangs. Not sure when this stopped working, need to investigate and fix this (and write a test).

Comment 1

[Mark Banner (:standard8)]

I've seen this occasionally when running ESLint. Although I think it might be a matter of timing - doing it at the wrong time hangs, but sometimes I can ctrl-c just fine.

Comment 2

[Andrew Halberstadt [:ahal]]

Hm yeah, it does seem to be exiting gracefully now.. though flake8 does take awhile (~5-10 seconds) to clean up after itself. When I filed this I was definitely seeing tracebacks on Ctrl-C though, which shouldn't be happening.

I guess I'll leave this open for now, maybe we'll run into it again and find reliable STR.

Comment 3

[Andrew Halberstadt [:ahal]]

This has regressed pretty badly with recent changes. Luckily I think I've managed to come up with a good fix.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1369711 - [mozlint] Refactor concurrent.futures ProcessPoolExecutor code for readability

Review commit: https://reviewboard.mozilla.org/r/222526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222526/

Comment 5

[Andrew Halberstadt [:ahal]]

I'm going to see if I can figure out an easy way to write a test for this tomorrow. Though I'm also going to be PTO for two weeks starting Monday, so might run out of time.

Either way, with this patch using Ctrl-C seems to work flawlessly (granted I'm scared to try Windows).

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1369711 - [mozlint] Make sure KeyboardInterrupts are handled well wherever they happen

There a few pieces needed here to properly handle KeyboardInterrupts.

1. All in-progress work needs to abort. Ideally the underlying linters will be
able to catch KeyboardInterrupt, and return partial results (like the flake8
linter does). Linters may alternatively allow the KeyboardInterrupt to
propagate up. Mozlint will catch and handle this appropriately, though any
results found will be lost. The only change to this behaviour was fixing a bug
in the flake8 linter.

2. Any unstarted jobs need to be canceled. In concurrent.futures, there are two
different queues. First, jobs are placed on the work queue, which is just a list
maintained by the parent process. As workers become available, jobs are moved
off the work queue, and onto the call queue (which is a multiprocessing.Queue).
Jobs that live on the work queue can be canceled with 'future.cancel()', whereas
jobs that live on the call queue cannot. The number of extra jobs that are stored
on the call queue is determined by this variable:
https://hg.mozilla.org/mozilla-central/file/deb7714a7bcd/third_party/python/futures/concurrent/futures/process.py#l86

In this patch, the parent process' sigint handler (which will be called on Ctrl-C)
is responsible for canceling all the jobs on the work queue. For the jobs on the
call queue, the best we can do is set a global variable that tells workers to
abort early.

3. Idle workers should exit gracefully. When there are no more jobs left, workers
will block on the call queue (either waiting for more jobs, or waiting for the
executor to send the shutdown signal). If a KeyboardInterrupt is received while a
worker is blocking, it isn't possible to intercept that anywhere (due to quirks
of how concurrent.futures is implemented). The InterruptableQueue class was
created to solve this problem. It will return None instead of propagating
KeyboardInterrupt. A None value will wake the worker up and tell it to gracefully
shutdown. This way, we avoid cryptic tracebacks in the output.

With all of these various pieces solved, pressing Ctrl-C appears to always exit
gracefully, sometimes even printing partial results.

Review commit: https://reviewboard.mozilla.org/r/227518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227518/

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8953237 [details]
Bug 1369711 - [mozlint] Refactor concurrent.futures ProcessPoolExecutor code for readability

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222526/diff/1-2/

Comment 8

[Andrew Halberstadt [:ahal]]

Hey gps, sorry I know you aren't familiar with this module at all, but for this series I thought it would be more important for the reviewer to be familiar with concurrent.futures. If you have suggestions for alternate reviewers feel free to change it.

Hopefully the commit messages/comments provide enough context. The first commit is precursor work to the second one (which is what actually fixes the bug). Let me know if you have any questions.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8953237 [details]
Bug 1369711 - [mozlint] Refactor concurrent.futures ProcessPoolExecutor code for readability

https://reviewboard.mozilla.org/r/222526/#review233922

This is a pretty large refactor. I wish it were smaller to make review easier.

I suspect there may be some lingering issues around process pool cleanup. These things are hard to get right.

But anyway, the refactor looks pretty straightforward. There's pretty good test coverage here. So that makes me feel a lot better that things didn't slip under the radar.

::: python/mozlint/mozlint/roller.py:185
(Diff revision 2)
>          # we're done adding to it.
>          paths = map(os.path.abspath, paths)
>  
>          num_procs = num_procs or cpu_count()
> -        all_results = defaultdict(list)
> -        with ProcessPoolExecutor(num_procs) as executor:
> +        jobs = list(self._generate_jobs(paths, num_procs))
> +

Removing the context manager here and not replacing with a ``try..finally`` makes me a little nervous. But I think things will be fine. There may be a tiny window where if a SIGINT or other interrupt comes in between creating the process pool and mucking with the signal handler.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8958610 [details]
Bug 1369711 - [mozlint] Make sure KeyboardInterrupts are handled well wherever they happen

https://reviewboard.mozilla.org/r/227518/#review233924

The code changes look fine.

There's likely a race condition in the test. I suppose not many people run these tests. Pretty much any test relying on `sleep()` will fail intermittently under some conditions. But if CI is happy, I'm happy.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8953237 [details]
Bug 1369711 - [mozlint] Refactor concurrent.futures ProcessPoolExecutor code for readability

https://reviewboard.mozilla.org/r/222526/#review233922

> Removing the context manager here and not replacing with a ``try..finally`` makes me a little nervous. But I think things will be fine. There may be a tiny window where if a SIGINT or other interrupt comes in between creating the process pool and mucking with the signal handler.

This was made better in the next commit. Yay!

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 8958610 [details]
Bug 1369711 - [mozlint] Make sure KeyboardInterrupts are handled well wherever they happen

https://reviewboard.mozilla.org/r/227518/#review233924

Yeah I don't like it either, but I couldn't think of any better way to test this :/

Unrelated to the sleep issue, I found out this test fails on Windows (sending signal.CTRL_C_EVENT doesn't seem to work as expected). I'll spend a bit of time trying to fix it, but may just end up disabling the test on Windows if it ends up being too difficult.

On Linux the test seems reliable.

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8958610 [details]
Bug 1369711 - [mozlint] Make sure KeyboardInterrupts are handled well wherever they happen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227518/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9712703bd242
[mozlint] Refactor concurrent.futures ProcessPoolExecutor code for readability r=gps
https://hg.mozilla.org/integration/autoland/rev/7fb1a832336d
[mozlint] Make sure KeyboardInterrupts are handled well wherever they happen r=gps

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/9712703bd242
https://hg.mozilla.org/mozilla-central/rev/7fb1a832336d