Skip to content

Commit

Permalink
bg_call_later: Enforce that scheduled callables are protected with @c…
Browse files Browse the repository at this point in the history
…aptures_crashes_to*
  • Loading branch information
davidfstr committed Feb 25, 2024
1 parent cf73ec1 commit bfd688c
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 5 deletions.
9 changes: 9 additions & 0 deletions src/crystal/browser/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from crystal.ui.BetterMessageDialog import BetterMessageDialog
from crystal.ui.log_drawer import LogDrawer
from crystal.ui.tree import DEFAULT_FOLDER_ICON_SET
from crystal.util.bulkheads import captures_crashes_to_stderr, captures_crashes_to
from crystal.util.finderinfo import get_hide_file_extension
from crystal.util.unicode_labels import decorate_label
from crystal.util.wx_bind import bind
Expand Down Expand Up @@ -613,6 +614,7 @@ def _on_download_entity(self, event) -> None:
# NOTE: It would be simpler to implement this logic with wx.Timer
# but wx.Timer seems to not work well if very many lambdas
# are scheduled with fg_call_later at the same time.
@captures_crashes_to_stderr
def elapsed_time_updater() -> None:
while True:
time.sleep(1.0)
Expand All @@ -629,6 +631,13 @@ def fg_task() -> bool:
else:
progress_dialog = None

# Run download() on a background thread because it can take a long time
# to instantiate the tree of related download tasks (when _LAZY_LOAD_CHILDREN == False)
#
# NOTE: Loudly crashes the entire scheduler thread upon failure.
# If this failure mode ends up happening commonly,
# suggest implementing a less drastic failure mode.
@captures_crashes_to(self.project.root_task)
def bg_task() -> None:
assert selected_entity is not None

Expand Down
2 changes: 2 additions & 0 deletions src/crystal/browser/entitytree.py
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,7 @@ def download_done(future: 'Future[ResourceRevision]') -> None:
f'Error downloading URL: {error_dict["type"]}: {error_dict["message"]}')
return

@captures_crashes_to(self)
def bg_task() -> None:
# Link parsing is I/O intensive, so do it on a background thread
try:
Expand Down Expand Up @@ -1302,6 +1303,7 @@ def fg_task_later() -> None:
return

self.update_children(force_populate=True)
@captures_crashes_to(self)
def bg_task():
# Give time for the loading node to display
time.sleep(.1)
Expand Down
4 changes: 4 additions & 0 deletions src/crystal/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -363,8 +363,12 @@ def _finish_launch(self, filepath: Optional[str]=None) -> None:
# Block until test-related modules are done loading,
# before starting bg_task() on background thread
from crystal.tests.index import run_tests
from crystal.util.bulkheads import captures_crashes_to_stderr
from crystal.util.xthreading import NoForegroundThreadError

# NOTE: Any unhandled exception will probably call os._exit(1)
# before reaching this decorator.
@captures_crashes_to_stderr
def bg_task():
# (Don't import anything here, because strange things can
# happen if the foreground thread tries to import the
Expand Down
1 change: 1 addition & 0 deletions src/crystal/tests/test_bulkheads.py
Original file line number Diff line number Diff line change
Expand Up @@ -1527,6 +1527,7 @@ async def _bg_call_and_wait(callable: Callable[[], _R], *, timeout: Optional[flo
timeout = _DEFAULT_WAIT_TIMEOUT_FOR_UNIT

result_cell = Future() # type: Future[_R]
@captures_crashes_to_stderr
def bg_task() -> None:
result_cell.set_running_or_notify_cancel()
try:
Expand Down
2 changes: 2 additions & 0 deletions src/crystal/tests/test_download.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from crystal.tests.util.tasks import wait_for_download_to_start_and_finish
from crystal.tests.util.wait import DEFAULT_WAIT_PERIOD, wait_for
from crystal.tests.util.windows import NewGroupDialog, OpenOrCreateDialog
from crystal.util.bulkheads import captures_crashes_to_stderr
from crystal.util.xthreading import bg_call_later
from http.server import BaseHTTPRequestHandler, HTTPServer
import io
Expand Down Expand Up @@ -487,6 +488,7 @@ def _send_route_response(self, route) -> None:
address = ('', self._port)
self._server = HTTPServer(address, RequestHandler)

@captures_crashes_to_stderr
def bg_task() -> None:
try:
self._server.serve_forever()
Expand Down
8 changes: 6 additions & 2 deletions src/crystal/util/bulkheads.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,13 @@ def run_bulkhead_call(
Raises AssertionError if the specified method is not actually
marked with @captures_crashes_to*.
"""
if getattr(bulkhead_call, '_captures_crashes', False) != True:
raise AssertionError(f'Expected callable {bulkhead_call!r} to be decorated with @captures_crashes_to*')
ensure_is_bulkhead_call(bulkhead_call)
return bulkhead_call(*args, **kwargs)


def ensure_is_bulkhead_call(callable: Callable) -> None:
if getattr(callable, '_captures_crashes', False) != True:
raise AssertionError(f'Expected callable {callable!r} to be decorated with @captures_crashes_to*')


# ------------------------------------------------------------------------------
10 changes: 7 additions & 3 deletions src/crystal/util/xthreading.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"""

from collections import deque
from crystal.util.bulkheads import captures_crashes_to_stderr, run_bulkhead_call
from crystal.util.bulkheads import captures_crashes_to_stderr, ensure_is_bulkhead_call
from crystal.util.profile import create_profiled_callable
from enum import Enum
from functools import partial, wraps
Expand Down Expand Up @@ -40,6 +40,9 @@
# Whether to enforce that callables scheduled with fg_call_later()
# be decorated with @captures_crashes_to*.
_DEFERRED_FG_CALLS_MUST_CAPTURE_CRASHES = True
# Whether to enforce that callables scheduled with bg_call_later()
# be decorated with @captures_crashes_to*.
_DEFERRED_BG_CALLS_MUST_CAPTURE_CRASHES = True


_P = ParamSpec('_P')
Expand Down Expand Up @@ -160,8 +163,7 @@ def fg_call_later(
is_fg_thread = is_foreground_thread() # cache

if _DEFERRED_FG_CALLS_MUST_CAPTURE_CRASHES:
callable = partial(run_bulkhead_call, callable, *args) # type: ignore[assignment]
args=()
ensure_is_bulkhead_call(callable)

if _PROFILE_FG_TASKS and profile and not is_fg_thread:
callable = create_profiled_callable(
Expand Down Expand Up @@ -328,6 +330,8 @@ def bg_call_later(
if True, forces the background thread to be a daemon,
and not prevent program termination while it is running.
"""
if _DEFERRED_BG_CALLS_MUST_CAPTURE_CRASHES:
ensure_is_bulkhead_call(callable)
thread = threading.Thread(target=callable, args=args, daemon=daemon)
thread.start()
return thread
Expand Down

0 comments on commit bfd688c

Please sign in to comment.