-
Notifications
You must be signed in to change notification settings - Fork 2k
[BUG] Do not leak tokio tasks in the log service. #4936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This PR ensures that background Tokio tasks spawned by the wal3 log service are properly terminated by implementing a Drop handler for LogWriter, which now calls the shutdown method on the inner writer upon dropping. This prevents resource leakage when a log is closed or dropped. This summary was automatically generated by @propel-code-bot |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
| let mut inner = self.inner.lock().unwrap(); | ||
| if let Some(writer) = inner.writer.as_mut() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[CompanyBestPractice]
Consider using parking_lot::Mutex instead of std::sync::Mutex in the LogWriter::drop implementation. Our company guideline states that parking_lot provides faster, more efficient implementations of synchronization primitives.
| let mut inner = self.inner.lock().unwrap(); | |
| if let Some(writer) = inner.writer.as_mut() { | |
| let mut inner = self.inner.lock(); | |
| if let Some(writer) = inner.writer.as_mut() { | |
| writer.shutdown(); | |
| } |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
377296f to
b69b94d
Compare
e7731ee to
3e0c82d
Compare
The background tasks of wal3 were leaked because the log service was not calling shutdown when dropping a log. This PR corrects that.
5b1283e to
ae3ed46
Compare
## Description of changes The background tasks of wal3 were leaked because the log service was not calling shutdown when dropping a log. This PR corrects that. ## Test plan Local benchmark. Will upload graph to GitHub PR. ## Documentation Changes N/A
## Description of changes Included changes - **[ENH] Purge dirty log in background at the end of scheduled compaction (#4915)** - **[ENH] Move Log GC to operator (#4919)** - **[BUG] Do not leak tokio tasks in the log service. (#4936)** - **[BUG] Log GC offset should be one above minimum compaction offset (#4938)** - **[ENH] Make roll dirty log always converge to coalesce everything. (#4927)** - **[BUG] Coalesce when multiple collections return the same info to compact (#4946)** - **[BUG] Enrich from the manifest if a cursor doesn't exist. (#4947)** - **[ENH] If the dirty log fails with LogContentionDurable, do not fail the operation. (#4953)** - **[ENH] Warn, not error, if dirty log has no cursor. (#4952)** - **[ENH] Cancellation safety for append_batch. (#4959)** ## Test plan CI ## Documentation Changes N/A --------- Co-authored-by: Macronova <[email protected]>
## Description of changes The background tasks of wal3 were leaked because the log service was not calling shutdown when dropping a log. This PR corrects that. ## Test plan Local benchmark. Will upload graph to GitHub PR. ## Documentation Changes N/A

Description of changes
The background tasks of wal3 were leaked because the log service was not
calling shutdown when dropping a log. This PR corrects that.
Test plan
Local benchmark. Will upload graph to GitHub PR.
Documentation Changes
N/A