Skip to content

fix: guard protocol job thread against DLL exceptions - #80

Merged
drewr95 merged 3 commits into
masterfrom
fix/76-protocol-job-thread-exception-guard
Sep 8, 2026
Merged

drewr95 merged 3 commits into
masterfrom
fix/76-protocol-job-thread-exception-guard

Conversation

@RaulSMS

@RaulSMS RaulSMS commented Aug 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • _protocol_job_thread in electronic_control_unit.py called self.j1939_dll.async_job_thread(now) with no exception handling. An uncaught exception there would fall out of the while loop and silently terminate the thread for the lifetime of the ECU, permanently stopping all TP/BAM timeout handling — with no log output indicating anything had gone wrong.
  • This is now guarded the same way the dispatch thread already guards notify(...) (logger.exception(...) + continue), consistent with the existing pattern in _timer_job_thread for callback exceptions.
  • On exception, next_wakeup falls back to now + 1.0 so the loop retries shortly instead of spinning or waiting indefinitely.

Fixes #76

Test plan

  • Added test_protocol_job_thread_survives_dll_exception in test/test_threading.py: monkeypatches async_job_thread to raise once, asserts the thread is still alive and gets called again afterward.
  • pytest test/test_threading.py -q — passes (one pre-existing, unrelated flaky thread-leak failure in test_dispatch_queue_drop_on_full reproduces identically on master, not introduced by this change).
  • pytest . --pyargs -q — 528 passed, same pre-existing failure.
  • ruff check on changed files — clean.

@RaulSMS

RaulSMS commented Aug 28, 2026

Copy link
Copy Markdown
Owner Author

Re #81: the CI failure on this PR turned out to have two separate causes, worth untangling.

  1. The actual bug (fixed in 351b403): my earlier commit on this branch accidentally deleted the ecu.stop() call from test_dispatch_queue_drop_on_full's finally block while inserting a new test right after it. That meant the test's ECU threads were never stopped at all — a real, deterministic leak, not the load-dependent flake Flaky test: test_dispatch_queue_drop_on_full occasionally fails the thread-leak fixture #81 describes. Restored the stop() call.

  2. Flaky test: test_dispatch_queue_drop_on_full occasionally fails the thread-leak fixture #81's proposed hardening (also applied here): while fixing that, I also added an explicit wait for all three ECU threads to exit (_wait_thread_exit(..., timeout=3.0)) instead of relying on ecu.stop()'s silent warn-only behavior when its internal join times out. This is exactly the "option 3" fix proposed in Flaky test: test_dispatch_queue_drop_on_full occasionally fails the thread-leak fixture #81, applied to this one test.

This does not fully close #81. The broader issue — conftest.py's _assert_no_j1939_thread_leak fixture only polling for 200ms globally, which could still produce false positives in other slow-callback tests that don't do an explicit join — is still open. Leaving #81 open for that.

khauersp
khauersp previously approved these changes Aug 28, 2026

@khauersp khauersp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe just a style preference here so I don't mind approving for now

Comment thread j1939/electronic_control_unit.py Outdated
RaulSMS added a commit that referenced this pull request Aug 28, 2026
Address review feedback on #80: name the DLL class and call that
raised, and state the retry behavior, so the log message alone tells
an operator what happened without needing to read the source.
@RaulSMS
RaulSMS requested a review from khauersp August 28, 2026 13:24
RaulSMS and others added 3 commits September 8, 2026 06:46
An unguarded exception from j1939_dll.async_job_thread() would fall out
of the while loop and silently kill _protocol_job_thread for the ECU's
lifetime, permanently stopping TP/BAM timeout handling. Catch and log
the exception the same way the dispatch and timer threads already do,
and retry after a short backoff instead of terminating.

Fixes #76

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The ecu.stop() call for this test's ECU was dropped when a prior commit
inserted a new test right after it, causing the ECU's three background
threads to leak past the end of the test and intermittently fail the
autouse thread-leak fixture in CI.

Also explicitly wait for all three threads to exit rather than relying
on stop()'s join-with-timeout, whose warn-only behavior on timeout can
otherwise race the (shorter) leak-detection window in conftest.py.

Fixes the CI failure reported on this PR; see #81 for the general
flakiness analysis this pattern guards against.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address review feedback on #80: name the DLL class and call that
raised, and state the retry behavior, so the log message alone tells
an operator what happened without needing to read the source.
@drewr95
drewr95 force-pushed the fix/76-protocol-job-thread-exception-guard branch from 4236872 to 72d2574 Compare September 8, 2026 10:46
@drewr95
drewr95 merged commit 3def3c1 into master Sep 8, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Protocol job thread has no exception guard around async_job_thread

3 participants