remove shutdown and add flush to Log trait #196

2017-06-05T14:55:04-05:00

pittma commented

2017-06-05 14:55:04 -05:00

(Migrated from github.com)

Fixes #117

mrhota commented

2017-06-06 11:19:54 -05:00

(Migrated from github.com)

@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.

@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 commented

2017-06-06 11:24:10 -05:00

(Migrated from github.com)

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

brson commented

2017-06-19 16:40:41 -05:00

(Migrated from github.com)

r? @sfackler

sfackler (Migrated from github.com) reviewed

2017-06-19 16:50:04 -05:00

src/lib.rs

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

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

pittma (Migrated from github.com) reviewed

2017-06-20 11:01:31 -05:00

src/lib.rs

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?

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?

sfackler (Migrated from github.com) reviewed

2017-06-20 11:22:13 -05:00

src/lib.rs

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.

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.

pittma (Migrated from github.com) reviewed

2017-06-20 11:35:08 -05:00

src/lib.rs

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

manuthambi (Migrated from github.com) reviewed

2017-06-28 18:10:05 -05:00

src/lib.rs

@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?

pittma (Migrated from github.com) reviewed

2017-06-28 18:31:27 -05:00

src/lib.rs

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.

manuthambi (Migrated from github.com) reviewed

2017-06-28 20:12:18 -05:00

src/lib.rs

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.

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.

sfackler (Migrated from github.com) reviewed

2017-06-28 20:26:32 -05:00

src/lib.rs

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

sfackler (Migrated from github.com) reviewed

2017-06-28 20:27:18 -05:00

src/lib.rs

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

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

brson commented

2017-07-05 21:07:48 -05:00

(Migrated from github.com)

Thanks for reviewing @sfackler.

pittma (Migrated from github.com) reviewed

2017-07-06 10:11:21 -05:00

src/lib.rs

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?

The [issue]( https://github.com/rust-lang-nursery/log/issues/117) 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`?

pittma (Migrated from github.com) reviewed

2017-07-06 10:12:43 -05:00

src/lib.rs

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

sfackler (Migrated from github.com) reviewed

2017-07-06 11:36:27 -05:00

src/lib.rs

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.

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 ```rust 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 commented

2017-07-14 19:51:23 -05:00

(Migrated from github.com)

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

@sfackler can you take another look?

Looks like this is ready to go. Thanks @danielscottt! @sfackler can you take another look?

sfackler (Migrated from github.com) reviewed

2017-07-14 20:24:03 -05:00

sfackler (Migrated from github.com) left a comment

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

src/lib.rs

					
				@ -196,3 +197,2 @@

				//! # extern crate log;

				//! # use log::{Level, LevelFilter, SetLoggerError, ShutdownLoggerError,

				//! #           Metadata};

				//! # use log::{Level, LevelFilter, Log, SetLoggerError, Metadata};