Skip to content
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

Drain on root logger should not require 'static. #95

Closed
ghost opened this issue Dec 19, 2016 · 4 comments
Closed

Drain on root logger should not require 'static. #95

ghost opened this issue Dec 19, 2016 · 4 comments

Comments

@ghost
Copy link

ghost commented Dec 19, 2016

Logger::root() requires drain: 'static, but it seems unnecessary.

pub struct Logger {
    drain: Arc<Drain<Error=Never>+Send+Sync>,
    values: OwnedKeyValueList,
}

Logger struct does not have a same constraint, and no impl depends on 'static
And I think root must be a root of one logger hierarchy, not root of global.

Example use case

Currently, I'm trying to capture logs as string to bypass rust-lang/rust#12309. As it must be done once per #[test] fn, buffer string is not 'static. I can do it via thread_local!, but it's quite hacky as it depends on bug.

---- definition::parser::tests::expansion_order_detection stdout ----
# I want to see logs for the test at here

@ghost
Copy link
Author

ghost commented Dec 19, 2016

I will do it soon, and I want to know if you don't mind removing 'static from root.

@dpc
Copy link
Collaborator

dpc commented Dec 20, 2016

I thought that + 'static there was to prevent Drain from having outside references. I'll be happy to review the change and then we can discuss as I'm a bit confused now.

@ghost
Copy link
Author

ghost commented Dec 20, 2016

I was wrong about 'static. Sorry for that.

And judging from my attempt to add lifetime to logger, it would break api compatability (if it's used in struct). I don't think it worth breaking all downstream crates. So I decided to leave slog as is, and implemented capture function using Arc<RwLock<Vec>>

let mut buf = Arc::new(RwLock::new(vec![]));

let _guard = PanicGuard { buf: buf.clone() };
let logger = init_slog(LogWriter { w: buf.clone() });

slog_scope::scope(logger, || op())

and it prints like (with color)

---- test name stdout ----
op: detecting expansion order
   DEBG [1] expr(1: g), depth: 2, max: 2
   DEBG [2] expr(2: g()), depth: 1, max: 2
   DEBG [0] expr(3: "test"), depth: 1, max: 2
   DEBG [3] expr(4: g()("test")), depth: 0, max: 2

(numbers in [] are not related to order of debug! call)

Arc is used because I can't take drain back.

My problem is solved, but leaving it open because I'm not sure if it should required 'static.
Please feel free to close it.

@ghost ghost changed the title Drain on root logger must not require 'static. Drain on root logger should not require 'static. Dec 20, 2016
@dpc
Copy link
Collaborator

dpc commented Dec 21, 2016

Ah, now I see what you were aiming at exactly.

So yes - Loggers share one Drain by reference counting and can use it from different threads, and anyone that has Logger can create it's clone or a sub-logger, so adding lifetime to loggers wouldn't generally work.

Closing it, but always feel free to reopen in case something could be improved.

@dpc dpc closed this as completed Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant