3Hren
Process fork support
Does blackhole support process forking? By support I mean ability to safely perform fork() and then use the "same" root logger in both parent and child processes. Probably sinks in the child process should reconnect all sockets, reopen files, etc, because it's a bad idea to send logs through the same socket from two processes.
Can you fix it? I've read through the code, and it seems that it's enough to remove all handlers from the root logger before fork. This will call destructors of all sinks and subsequently flush logs/join threads/close files/etc (at least for the standard sinks). So if there were methods to remove all handlers and configure them again after fork, it would be enough to support forking. Or I'm missing something?
Then it's enough to reassign the logger just before/after fork, which you can do explicitly without library support. Blackhole supports thread-safe reassignments using operator=(&&).
Seriously, I haven't seen any library that was OK with forking without doing black magic around.
I'm not sure what happens with logger's scope::manager_t after assignment. Seems to me that if I assign to the root logger while there is any watcher_t object, it will lose the scoped attributes (even segfault?).
Seriously, nginx does fork. Is this library not supposed to be used in programs like nginx?
It's not about testing, it's about how it's supposed to be. But anyway, I wrote a test:
root_logger_t logger(std::move(handlers1));
const scope::holder_t s1(logger, {{"key#1", {42}}});
{
const scope::holder_t s2(logger, {{"key#2", {"value#2"}}});
logger.log(0, "paryu"); // attributes: key#1 and key#2
logger = root_logger_t(std::move(handlers2));
logger.log(0, "gde"); // not attrs
}
logger.log(0, "hochu"); // attributes: key#1
Looks bad. Some other way to remove/restore handlers is required.
Also it's not enough to just make something that works. The point is whether the library declares support of forking or not. Because if it's not guaranteed that the reassignment approach will continue to work, it's not useful. Now I'm trying to tell you that it's relatively easy to implement this feature and it would be great if you did it and declared support of forking in the documentation.
Looks bad.
Looks as expected: https://github.com/3Hren/blackhole/blob/master/tests/root.cpp#L355 Maybe for someone it's counterintuitive, but that's how it was implemented and I cannot break API right now.
Some other way to remove/restore handlers is required.
Ideas? What to do with scoped attributes in that case?
Now I'm trying to tell you that it's relatively easy to implement this feature and it would be great if you did it and declared support of forking in the documentation.
I'd happily declare fork-safety support when it will be done. But right now I have no idea how to implement this properly, I don't know how computers work, sorry. I know the man, who knows - @andrusha97!
Looks as expected
Looks bad if I want to assign to the logger before and after fork. I'm not criticizing your design :)
Ideas? What to do with scoped attributes in that case?
How about method root_logger_t::reset_handlers(std::vector<std::unique_ptr<handler_t>> new_handlerz) which will reset handlers without touching watchers?
I don't know how computers work, sorry
Why sarcasm? It's a sirious issue!
@andrusha97!
Nice ava.
Adding such method seems ok. But who creates handlers before passing to the method?
It's a hard question. More methods are needed:
-
void root_logger_t::reset_filter(filter_t) -
void builder_t::configure(root_logger_t &)- should configure logger usingreset_filterandreset_handlers.
Well, filters can be reset right now. Handlers are not. May be just add a method, which destructs current logger into handlers?
Sorry for my Rust:
fn into_handlers(self) -> Vec<Box<Handler>>;
...
let handlers = logger.into_handlers();
It looks unnatural to me. If I didn't know about this particular case, I couldn't tell where this method might be used.
Also I realised that there's another common use case for such configure method - configuration reload on SIGHUP.
Also I realised that there's another common use case for such configure method - configuration reload on SIGHUP.
Yep, currently we do this with operator=(&&), seems like with possible scoped attributes losing.