-
Notifications
You must be signed in to change notification settings - Fork 15
refactor: call flush_atexit at exit #102
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
base: main-dev
Are you sure you want to change the base?
Conversation
Signed-off-by: tison <[email protected]>
pub(crate) fn flush_sinks_atexit(&self) { | ||
self.sinks.iter().for_each(|sink| { | ||
if let Err(err) = sink.flush_atexit() { | ||
self.handle_error(err); | ||
} | ||
}); | ||
} | ||
|
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.
This may be even pub
if users set their own global static LOGGER. But given that is not quite possible it's OK to leave pub(crate)
until real use cases occur.
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.
I like this idea! In fact, previously we still couldn't guarantee that users wouldn't use thread-local (or anything else that might conflict with atexit()
) in their own sinks, because the global atomic bool variable IS_TEARING_DOWN
is not public. This PR enables users to be aware that the program is in an atexit()
callback and customize the flush implementation for this case.
Perhaps one minor nitpick is that I'm not sure if there might be a better choice for the naming. flush_atexit
implies that it will be called in the atexit()
callback, which is good, on the other hand the naming style is not so Rusty... I'll think about it some more, anyway it's not a big issue.
// before we do that we have to destroy the thread pool to ensure that any | ||
// pending log tasks are completed. | ||
self.thread_pool.destroy(); | ||
self.backend.flush() |
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.
We should also flush the underlying sub-sinks with flush_atexit
method too.
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.
IIUC self.backend.flush()
already does it?
This is an identical refactor from the previous code snippet.
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.
The point here is that sub-sinks are agnostic to AsyncPoolSink
. If a sub-sink is user-defined, it may still use thread-local in its flush()
, and it won't know if the program is tearing down.
In other words, as a combined sink, I think AsyncPoolSink
should forward flush_atexit
to its sub-sinks, so that it can propagate the information that "this flush is the last flush, you are currently in the atexit
callback".
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.
Currently, self.backend.flush()
is still calling .flush()
for sub-sinks.
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.
Ah. I got it.
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.
self.backend.flush() | |
self.backend.flush_atexit() |
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.
And please also apply a flush_atexit
override for DedupSink
, as it's also a conbined-sink.
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.
Resolved at 3e52f81. Although the name looks more terrible.
Yeah. I'm struggling with naming this method also. Let's give it some days for potential new ideas :D |
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
pub(crate) fn flush_sinks_atexit(&self) { | ||
self.sinks.iter().for_each(|sink| { | ||
if let Err(err) = sink.flush_atexit() { | ||
self.handle_error(err); | ||
} | ||
}); | ||
} |
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.
Nitpick, for better maintainability
fn flush_sinks_with<'a>(&'a self, with: impl Fn(&'a dyn Sink) -> Result<()>) {
self.sinks.iter().for_each(|sink| {
if let Err(err) = with(&**sink) {
self.handle_error(err);
}
});
}
fn flush_sinks_atexit(&self) {
self.flush_with(Sink::flush_atexit);
}
fn flush_sinks(&self) {
self.flush_with(Sink::flush);
}
Same thing for AsyncPoolSink
and DedupSink
.
Avoid global static
IS_TEARING_DOWN
trick.