Skip to content

Commit

Permalink
Fix rare 'no python frame' by not starting knocker thread in testing
Browse files Browse the repository at this point in the history
  • Loading branch information
milesgranger committed Apr 4, 2023
1 parent d756f34 commit 1c560df
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 4 deletions.
33 changes: 33 additions & 0 deletions conftest.py
@@ -1,6 +1,8 @@
# https://pytest.org/latest/example/simple.html#control-skipping-of-tests-according-to-command-line-option
from __future__ import annotations

from unittest import mock

import pytest

# Uncomment to enable more logging and checks
Expand Down Expand Up @@ -39,4 +41,35 @@ def pytest_collection_modifyitems(config, items):
item.add_marker(pytest.mark.workerstate)


@pytest.fixture(autouse=True)
def patch_gilknocker(request):
"""
A very subtle bug where a SIGABRT can kill the main Python thread out
from underneath gilknocker's GIL sampling thread which can lead to:
FATAL: exception not rethrown
Fatal Python error: Aborted
Current thread 0x00007fbae56bc640 (most recent call first):
<no Python frame>
Due to a test/object/ect not properly calling 'gilknocker.KnockKnock.stop'.
This bug rarely shows up as is, even when trying to bring it up deliberatly.
More details can be found here: https://github.com/PyO3/pyo3/issues/2102
Therefore, we patch all tests so `start` doesn't trigger the sampling thread,
except when explicitly tested in 'test_gil_contention' test.
"""
if "gil" in request.node.name:
yield
else:
try:
import gilknocker # noqa F401
except ImportError:
yield
else:
with mock.patch("gilknocker.KnockKnock.start"):
yield


pytest_plugins = ["distributed.pytest_resourceleaks"]
5 changes: 1 addition & 4 deletions distributed/system_monitor.py
Expand Up @@ -229,7 +229,4 @@ def range_query(self, start: int) -> dict[str, list]:

def close(self) -> None:
if self.monitor_gil_contention:
try:
self._gilknocker.stop()
except ValueError: # Wasn't started or already stopped
pass
self._gilknocker.stop()

0 comments on commit 1c560df

Please sign in to comment.