-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[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