Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where coverage data is not saved on SIGTERM #1600

Merged
merged 3 commits into from
Apr 6, 2023
Merged

Fix bug where coverage data is not saved on SIGTERM #1600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f04500c
Add test that reproduces the issue
LewisGaul Apr 2, 2023
2908b78
Suggested fix - always save data in sigterm exit flow
LewisGaul Apr 2, 2023
ba8c6c0
Address test failures on MacOS due to lack of 'Terminated' output on …
LewisGaul Apr 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion coverage/control.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ def _atexit(self, event: str = "atexit") -> None:
self._debug.write(f"{event}: pid: {os.getpid()}, instance: {self!r}")
if self._started:
self.stop()
if self._auto_save:
if self._auto_save or event == "sigterm":
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why _auto_save isn't true, but I've only just taken a quick look at this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wondered that at first - it's being set by the auto_data arg to the Coverage class, which defaults to False and is passed as True by multiproc.ProcessWithCoverage, i.e. only in the multiprocessing case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I'm more than happy with solving this another way - more than anything this proposed change serves as an illustration for what's going wrong!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to think this through. On the one hand it seems odd: if _auto_save is false, then we won't save the data if the process ends normally, but we will if it's terminated? But on the other, sigterm=true is documented as "to capture data at unusual process ends," which this definitely is.

Maybe the answer is that _auto_save is only needed for the subprocess case. For main processes, something else will be handling the data saving at normal process termination, and this is fine to put here.

I like that you have a test demonstrating the issue. event == "sigterm" will only happen if sigterm=true is in the settings, so clearly the user wanted sigterm handling.

Let's merge it!

self.save()

def _on_sigterm(self, signum_unused: int, frame_unused: Optional[FrameType]) -> None:
Expand Down
27 changes: 26 additions & 1 deletion tests/test_concurrency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ class SigtermTest(CoverageTest):
"""Tests of our handling of SIGTERM."""

@pytest.mark.parametrize("sigterm", [False, True])
def test_sigterm_saves_data(self, sigterm: bool) -> None:
def test_sigterm_multiprocessing_saves_data(self, sigterm: bool) -> None:
# A terminated process should save its coverage data.
self.make_file("clobbered.py", """\
import multiprocessing
Expand Down Expand Up @@ -751,6 +751,31 @@ def subproc(x):
expected = "clobbered.py 17 5 71% 5-10"
assert self.squeezed_lines(out)[2] == expected

def test_sigterm_threading_saves_data(self) -> None:
# A terminated process should save its coverage data.
self.make_file("handler.py", """\
import os, signal

print("START", flush=True)
print("SIGTERM", flush=True)
os.kill(os.getpid(), signal.SIGTERM)
print("NOT HERE", flush=True)
""")
self.make_file(".coveragerc", """\
[run]
# The default concurrency option.
concurrency = thread
sigterm = true
""")
out = self.run_command("coverage run handler.py")
if env.LINUX:
assert out == "START\nSIGTERM\nTerminated\n"
else:
assert out == "START\nSIGTERM\n"
out = self.run_command("coverage report -m")
expected = "handler.py 5 1 80% 6"
assert self.squeezed_lines(out)[2] == expected

def test_sigterm_still_runs(self) -> None:
# A terminated process still runs its own SIGTERM handler.
self.make_file("handler.py", """\
Expand Down