fix: make EventWriter thread-safe to prevent event loss during cleanup#55
fix: make EventWriter thread-safe to prevent event loss during cleanup#55
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThread-safety synchronization is added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
=======================================
Coverage 88.47% 88.47%
=======================================
Files 48 48
Lines 4684 4684
=======================================
Hits 4144 4144
Misses 540 540 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
snakemake-logger-plugin-snakesee/src/snakemake_logger_plugin_snakesee/writer.py (1)
114-123: Consider: Exception during flush could leak the file handle.If
_flush_locked()raises an exception (e.g., disk full, I/O error),_closedis alreadyTruebutself._fileis never closed. Subsequent calls toclose()will return early due to the idempotency check, leaving the file handle leaked.🛡️ Proposed fix using try/finally
def close(self) -> None: """Close the file and flush any remaining events.""" with self._lock: if self._closed: return self._closed = True - self._flush_locked() - if self._file is not None: - self._file.close() - self._file = None + try: + self._flush_locked() + finally: + if self._file is not None: + self._file.close() + self._file = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakemake-logger-plugin-snakesee/src/snakemake_logger_plugin_snakesee/writer.py` around lines 114 - 123, The close method sets self._closed before calling _flush_locked, so if _flush_locked raises the file handle (self._file) can be leaked; update Writer.close to call _flush_locked inside a try/finally (or similar) so that regardless of exceptions you still close and set self._file to None and preserve idempotency: call _flush_locked() in the try, and in the finally block check and close self._file and set it to None; ensure you still set self._closed (or only set it after successful/guaranteed cleanup depending on desired semantics) and keep the locking around these operations to avoid races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@snakemake-logger-plugin-snakesee/src/snakemake_logger_plugin_snakesee/writer.py`:
- Around line 114-123: The close method sets self._closed before calling
_flush_locked, so if _flush_locked raises the file handle (self._file) can be
leaked; update Writer.close to call _flush_locked inside a try/finally (or
similar) so that regardless of exceptions you still close and set self._file to
None and preserve idempotency: call _flush_locked() in the try, and in the
finally block check and close self._file and set it to None; ensure you still
set self._closed (or only set it after successful/guaranteed cleanup depending
on desired semantics) and keep the locking around these operations to avoid
races.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 170a1d92-72a4-408c-9926-de9d94a7c07c
📒 Files selected for processing (2)
snakemake-logger-plugin-snakesee/src/snakemake_logger_plugin_snakesee/writer.pysnakemake-logger-plugin-snakesee/tests/test_writer.py
Snakemake's _cleanup() calls handler.close() from the main thread while the QueueListener background thread may still be delivering events via emit(). This race condition causes a ValueError (I/O on closed file) that crashes the QueueListener thread, silently dropping remaining events. Add a threading.Lock and _closed flag to EventWriter so that close() and write() are mutually exclusive. Writes after close() are silently ignored instead of crashing. close() is now idempotent. See: snakemake/snakemake#4136
nh13
force-pushed
the
nh/thread-safe-event-writer
branch
from
7fd2b45 to
9d507b8
Compare
1 participant
Summary
threading.Lockand_closedflag toEventWriterso thatclose()andwrite()are mutually exclusiveclose()are silently ignored instead of crashing the QueueListener threadclose()is now idempotent (safe to call multiple times)Root cause: Snakemake's
_cleanup()callshandler.close()from the main thread while theQueueListenerbackground thread may still be delivering events viaemit(). Without thread safety, this race causes aValueError(I/O on closed file) that crashes the background thread, silently dropping remaining events. Filed upstream as snakemake/snakemake#4136.Test plan
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests