fix: harden MultiprocessingWriter shutdown and error handling - #241
fix: harden MultiprocessingWriter shutdown and error handling#241bednar wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
- Coverage 87.43% 86.19% -1.25%
==========================================
Files 28 28
Lines 2030 2072 +42
==========================================
+ Hits 1775 1786 +11
- Misses 255 286 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
277db3d to
6184925
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new unit tests hard-code start_method="fork" and assert fork, which is non-portable (e.g., Windows) and inconsistent with the library default and existing integration coverage expecting spawn.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens MultiprocessingWriter’s lifecycle management and worker-loop error handling to avoid inconsistent shutdown states and JoinableQueue.join() deadlocks, while adding regression tests and documenting the behavior change.
Changes:
- Replaces assertion-based state checks with explicit
RuntimeErrors and adds an idempotentclose()with bounded shutdown (close_timeout) plus best-effort__del__cleanup. - Ensures
queue.task_done()is always called for dequeued items (including write failures), and makeson_shutdownrun at most once. - Adds regression tests for lifecycle validation, worker timeout, poison-pill shutdown, and callback/write failures; documents the change in the changelog.
File summaries
| File | Description |
|---|---|
influxdb_client_3/write_client/client/util/multiprocessing_helper.py |
Implements the hardened shutdown lifecycle, deterministic worker cleanup behavior, and callback/error handling improvements. |
tests/test_multiprocessing_helper.py |
Adds unit/regression coverage for the updated lifecycle and worker behavior (with a portability concern noted in comments). |
CHANGELOG.md |
Documents the MultiprocessingWriter shutdown/error-handling hardening. |
Review details
Suppressed comments (1)
tests/test_multiprocessing_helper.py:121
- This assertion is tied to the test helper's hard-coded
forkstart method; if the helper is updated to use the library default (spawn) for portability, this expectation should be updated accordingly.
def test_get_start_processing_method_returns_context_method():
assert make_writer().get_start_processing_method() == "fork"
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new unit test test_get_start_processing_method_returns_context_method asserts "fork" despite constructing the writer with start_method="spawn", which will cause test failures.
Review details
Suppressed comments (1)
tests/test_multiprocessing_helper.py:122
- This test expects the start method to be "fork", but
make_writer()explicitly constructsMultiprocessingWriter(start_method="spawn", ...), soget_start_processing_method()should return "spawn". As written, the test will fail (and also disagrees with existing integration tests that expect "spawn" by default).
def test_get_start_processing_method_returns_context_method():
assert make_writer().get_start_processing_method() == "fork"
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: bednar <455137+bednar@users.noreply.github.com>
Closes #230
Proposed Changes
Harden
MultiprocessingWriterlifecycle and worker error handling:close()with bounded worker shutdown and configurableclose_timeout.CHANGELOG.md.Checklist