remove shutdown and add `flush` to `Log` trait #196

pittma · Jun 5, 2017

Fixes #117

mrhota · Jun 6, 2017

@danielscottt Wasn't main idea of #117 to remove shutdown? I thought exposing flush functionality was more of a since-we're-in-the-neighborhood sort of thing.

pittma · Jun 6, 2017

@mrhota It is quite possible that I misunderstood the issue altogether.

brson · Jun 19, 2017

r? @sfackler

sfackler · Jun 19, 2017

sfackler reviewed Jun 19, 2017

View changes

sfackler · Jun 19, 2017

src/lib.rs

@@ -1153,12 +1155,6 @@ mod panic {

 struct LoggerGuard(&'static Log);

-impl Drop for LoggerGuard {


This synchronization needs to stick around, since Log isn't an unsafe trait.

What would the implementation of Drop synchronize on for LoggerGuard? Is the idea to synchronize all __log calls around flush? Is that how the need for shutdown is removed?

The refcounting logic ensures there aren't any log calls actively using the logger while we tear it down, which would be a use-after-free. We can't depend on flush doing this synchronization for us - the default implementation does nothing at all for example!

We're removing shutdown because it's not sufficiently useful compared to its cost in all of this refcounting. The addition of flush is only semi-related - it's just a way to allow people to make sure that any buffers or whatever are flushed before their application exits so they don't drop messages on the floor.

Thanks for the guidance, here! As I feared, I fundamentally misunderstood the ask. I'll clean this up and repush shortly.

@sfackler Perhaps I am totally confused, but why would you need that synchronization? Without shutdown, the logger is never deallocated (or dropped), so we are never going to tear it down?

This is why I left it here after I refactored my first pass, which misunderstood the task. But on account of my initial confusion, I can certainly imagine that I'm missing something.

Also, I think the load/stores from STATE only need Ordering::Acquire/Release not SeqCst. Using SeqCst synchronizes with any unrelated SeqCst read/writes somewhere else in the program.

Oh, did I totally miss the fact that this PR also removed shutdown? Oops >_>

sfackler · Jun 29, 2017

sfackler reviewed Jun 29, 2017

View changes

sfackler · Jun 29, 2017

src/lib.rs

@@ -716,6 +692,9 @@ pub trait Log: Sync + Send {
    /// Implementations of `log` should perform all necessary filtering
    /// internally.
    fn log(&self, record: &Record);
+
+    /// Flushes any buffered records.
+    fn flush(&self) {}


We should maybe not have a default implementation here? Write::flush does't have a default as a comparison.

The issue mentioned flush ought to have a default implementation, but I'm happy to remove it here. This of course has ramifications for env_logger; would its implementation be expected to use the same reference counting pattern previously used here when coordinating on shutdown?

I'll go ahead and prep a patch that does this in the meantime.

I think the issue mentioned a default impl to retain back compat, which we don't really need to worry about since we're removing shutdown at the same time.

I don't think we'd want to add the refcounting into the flush impl for env_logger - it'd defeat the purpose of getting rid of shutdown in the first place! The use case for flush is when you're for example, in the shutdown process of your application. If you use something like env_logger then just killing the process is fine - any log calls that finished before the exit will end up on the console. But, if you're using a logger that performs the IO asynchronously, you can end up in situations where log lines that should have been output aren't. Imagine something like

info!("shutting down"); log::flush(); process::exit(0);

All we need to make sure is that the "shutting down" message makes it all the way through the logging infrastructure. If there are other threads doing things in the background, the state of their messages is not super important since they're racing with this thread.

brson · Jul 6, 2017

Thanks for reviewing @sfackler.

brson · Jul 15, 2017

Looks like this is ready to go. Thanks @danielscottt!

@sfackler can you take another look?

sfackler · Jul 15, 2017

sfackler reviewed Jul 15, 2017

View changes

Just a couple of tiny tweaks and I think this is good to go!

sfackler · Jul 15, 2017

src/lib.rs

-//!         let logger = unsafe { &*(logger as *const SimpleLogger) };
-//!         logger.flush();
-//!     })
+//! pub fn shutdown() {


Let's just remove this bit of the example.

sfackler · Jul 15, 2017

src/lib.rs

@@ -1178,12 +1092,6 @@ mod panic {

 struct LoggerGuard(&'static Log);


This type can be removed entirely now that we don't need a destructor, but that can be handled later.

sfackler · Jul 15, 2017

src/lib.rs

@@ -715,6 +696,9 @@ pub trait Log: Sync + Send {
    /// Implementations of `log` should perform all necessary filtering
    /// internally.
    fn log(&self, record: &Record);
+
+    /// Flushes any buffered records.
+    fn flush(&self) {}


Looks like we still need to remove the default impl here.

aturon · Aug 2, 2017

Ping @sfackler

sfackler · Aug 10, 2017

Thanks!

sfackler · Aug 10, 2017

Sorry for the very slow review cycle D:

pittma force-pushed the pittma:flush branch from 6ab6756 to c3ded0a Jun 5, 2017

pittma force-pushed the pittma:flush branch from c3ded0a to 1ed8201 Jun 20, 2017

pittma changed the title ~~add `flush` to `Log` trait~~ remove shutdown and add `flush` to `Log` trait Jun 20, 2017

pittma force-pushed the pittma:flush branch 3 times, most recently from 5ddba76 to 4aa4c9d Jul 6, 2017

pittma force-pushed the pittma:flush branch from 4aa4c9d to e66665f Jul 17, 2017

removes shutdown and adds `flush` to Log trait.
Signed-off-by: dan pittman <danielscottt@gmail.com>

Unverified

No user is associated with the committer email.

Learn about signing commits

Loading status checks…

44bbdbd

pittma force-pushed the pittma:flush branch from e66665f to 44bbdbd Jul 17, 2017

sebasmagri referenced this pull request Aug 4, 2017
Closed
Remove env_logger from this repository #145
2 of 2 tasks complete

sfackler merged commit 4fb1ee8 into rust-lang-nursery:master Aug 10, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

mjkillough added a commit to mjkillough/env_logger that referenced this pull request Aug 21, 2017

Implement `Logger::flush()` for `Log`.
Same as the changes to `env_logger` in rust-lang-nursery/log#196.

5814a73

mjkillough referenced this pull request Aug 21, 2017
Merged
Catch up with changes to env_logger in rust-lang-nursery/log #15

mjkillough added a commit to mjkillough/env_logger that referenced this pull request Aug 21, 2017

Implement `Logger::flush()` for `Log`.
Same as the changes to `env_logger` in rust-lang-nursery/log#196.

91e6929

rust-lang-nursery/log

Join GitHub today

remove shutdown and add `flush` to `Log` trait #196

Conversation

pittma commented Jun 5, 2017 • edited

pittma force-pushed the pittma:flush branch from 6ab6756 to c3ded0a Jun 5, 2017

This comment has been minimized.

mrhota commented Jun 6, 2017

This comment has been minimized.

pittma commented Jun 6, 2017

This comment has been minimized.

brson commented Jun 19, 2017

sfackler reviewed Jun 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pittma force-pushed the pittma:flush branch from c3ded0a to 1ed8201 Jun 20, 2017

pittma changed the title add `flush` to `Log` trait remove shutdown and add `flush` to `Log` trait Jun 20, 2017

sfackler reviewed Jun 29, 2017 View changes

This comment has been minimized.

This comment has been minimized.

pittma Jul 6, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brson commented Jul 6, 2017

pittma force-pushed the pittma:flush branch 3 times, most recently from 5ddba76 to 4aa4c9d Jul 6, 2017

This comment has been minimized.

brson commented Jul 15, 2017

sfackler reviewed Jul 15, 2017 View changes

sfackler left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pittma force-pushed the pittma:flush branch from 4aa4c9d to e66665f Jul 17, 2017

pittma force-pushed the pittma:flush branch from e66665f to 44bbdbd Jul 17, 2017

This comment has been minimized.

aturon commented Aug 2, 2017

sebasmagri referenced this pull request Aug 4, 2017

This comment has been minimized.

sfackler commented Aug 10, 2017

This comment has been minimized.

sfackler commented Aug 10, 2017

Hide details View details sfackler merged commit 4fb1ee8 into rust-lang-nursery:master Aug 10, 2017 2 checks passed

2 checks passed

mjkillough added a commit to mjkillough/env_logger that referenced this pull request Aug 21, 2017

mjkillough referenced this pull request Aug 21, 2017

mjkillough added a commit to mjkillough/env_logger that referenced this pull request Aug 21, 2017

pittma commented Jun 5, 2017 •

edited

pittma force-pushed the pittma:flush branch from `6ab6756` to `c3ded0a` Jun 5, 2017

sfackler reviewed Jun 19, 2017

View changes

pittma force-pushed the pittma:flush branch from `c3ded0a` to `1ed8201` Jun 20, 2017

sfackler reviewed Jun 29, 2017

View changes

pittma Jul 6, 2017 •

edited

pittma force-pushed the pittma:flush branch 3 times, most recently from `5ddba76` to `4aa4c9d` Jul 6, 2017

sfackler reviewed Jul 15, 2017

View changes

pittma force-pushed the pittma:flush branch from `4aa4c9d` to `e66665f` Jul 17, 2017

pittma force-pushed the pittma:flush branch from `e66665f` to `44bbdbd` Jul 17, 2017

sfackler merged commit `4fb1ee8` into rust-lang-nursery:master Aug 10, 2017
2 checks passed