Closed Bug 1273556 Opened 5 years ago Closed 5 years ago

[mozlint] Handle SIGINT better and still display partial results

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ahal, Assigned: ahal)

Details

Attachments

(1 file)

The SIGINT (KeyboardInterrrupt) handling in mozlint is currently broken. A python bug is causing SIGINT to get swallowed, making it impossible to Ctrl-C |mach lint|.

While fixing this, I came up with a better idea. Get the parent process to ignore SIGINT, and handle it cleanly in the child processes. This way, we can still collect and display whatever results are already finished. If the external linter is implemented properly (i.e results are streamed), we can even return partial results from a specific linter.
Currently a bug in python (https://bugs.python.org/issue8296) is preventing a KeyboardInterrupt from
reaching the parent process, meaning we can't kill the process with SIGINT. There is a workaround to
this bug, but instead I decided to ignore SIGINT in the parent process completely. Now, each child
process is responsible for handling SIGINT on its own. Since child processes should all shutdown
relatively quickly anyway, this effectively also ends the parent process.

The benefit of doing it this way is that each child process can return the results they have collected
to date. So when a developer hits Ctrl-C, they'll still see some (but not all) formatted lint output.
The downside is that a poorly implemented external linter could block the parent process from exiting
quickly, but if this happens we should just fix the linter.

Review commit: https://reviewboard.mozilla.org/r/53310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53310/
Attachment #8753473 - Flags: review?(james)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Attachment #8753473 - Flags: review?(james) → review+
Comment on attachment 8753473 [details]
MozReview Request: Bug 1273556 - [mozlint] Better SIGINT handling, return partial results on Ctrl-C, r?jgraham

https://reviewboard.mozilla.org/r/53310/#review50250

I guess. The lack of any timeout in the parent process causing it to forcibly shut down if the child processes don't return after a SIGINT makes me rather nervious though.

::: python/mozlint/mozlint/types.py:42
(Diff revision 1)
>          if self.batch:
>              return self._lint(paths, linter, **lintargs)
>  
>          errors = []
> +        try:
> -        for p in paths:
> +            for p in paths:

Tabs
Good point, filed bug 1273914.
https://reviewboard.mozilla.org/r/53310/#review50250

> Tabs

Hm, I don't see any tabs here?
https://reviewboard.mozilla.org/r/53310/#review50250

> Hm, I don't see any tabs here?

Oh, i misunderstood reviewboard's notation it seems.
https://hg.mozilla.org/mozilla-central/rev/0f9e19a123e3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: General → Lint
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.