wonder-mice / zf_log

Core logging library for C/ObjC/C++
MIT License
194 stars 51 forks source link

thread safety ? #16

Closed fenugrec closed 7 years ago

fenugrec commented 7 years ago

Hi, I'm interested in this library but have a few questions regarding thread safety. The readme says it is "thread-safe", but what happens when a thread writes a log message while the output callback is already in progress (called from a another thread) ? I don't see any explicit locking / queuing in the built-in output callbacks...

I know there has been some testing with multiple threads, so maybe I'm just missing something ? I'd like to hear more about this.

Thanks!

wonder-mice commented 7 years ago

Hi, good question. Library doesn't have any explicit locking around output callback, because not all output callbacks need one. So, if output callback needs any synchronization it can provide its own.

stderr with WriteFile (win) : how can you tell if FILE_APPEND_DATA or FILE_WRITE_DATA are set ?

You can verify with GetFileInformationByHandleEx() or something like that, that returned handle has FILE_APPEND_DATA set. And if you think about that, it should, since you can't seek() stderr anyway. Yes, they don't document that explicitly, but since it's the only sane thing to do I'm OK with relying on it in default implementation. Once there will be a windows version that doesn't do that, we can think about what can be done about that. But I would agree that unit test that checks for that would be nice.

stderr with write() (*nix)

Experience shows that it's atomic for most Unix flavors you can find around. When some *nix will be found where it's not atomic, it can be addressed specifically. I would prefer to have fast code for common case and additional locking for outliers.

See this post from Linus (search for "with a chainsaw laughing maniacally"): http://yarchive.net/comp/linux/wakekill.html Also see this SO and links from it: http://stackoverflow.com/questions/12942915/understanding-concurrent-file-writes-from-multiple-processes/12943431#12943431

OutputDebugString

It better be async safe, otherwise it's unusable. OutputDebugString() can be used from different subsystems (like 3rd-party libraries) that have no idea about each other. No way they can synchronize properly.

So, to sum up. Probably you can find some formal loop holes that can indicate that writes possibly can be non-atomic, but reality is that they are. If there will be a need to use this library on system where writes are non-atomic we can fix library specifically for this system (maybe even without introducing a lock, since we will know what we are dealing with).

fenugrec commented 7 years ago

Hi, thanks for the detailed answers; all valid points.

Are you aware if anyone has made an output callback that queues + buffers writes to a logfile ? Basically a FIFO so the callback return almost immediately without waiting on file IO.

This wasn't really an "issue" anyway, just a question (wasn't sure how else to contact you), so I guess I can close this.

Cheers

wonder-mice commented 7 years ago

Are you aware if anyone has made an output callback that queues + buffers writes to a logfile ? Basically a FIFO so the callback return almost immediately without waiting on file IO.

No, there is none AFAIK. It's not a trivial thing to do though. The hardest question is how much to buffer and what to do when logs are going in faster when IO is capable of writing them out. In the end, you either will have a chance to block anyway or will drop (newer or older) log lines when buffer is full. And to be honest, write() already does exactly that. It doesn't wait for physical IO to be completed. Though because of sys call involved overhead could be suboptimal, yes.

In general, I think it would be nice to have a companion library with most popular output callback implementations. It's just nobody needed one so far...

fenugrec commented 7 years ago

Indeed, it adds a failure point in the case of buffer overrun. In my case I could deal with an occasional blocking call, but I think either behavior could be OK, as long as it's documented.

I'm considering porting the buffering code from "zlog" ( https://github.com/zma/zlog/blob/master/zlog.c ) to zf_log for my own use (with some modifications to use wrapper functions around pthread / winAPI mutexes). If you have ideas on how to integrate it nicely enough for this project, I'd be glad to eventually submit a PR.

I'm thinking your "companion library" could just be an extra set of .c files that contain a bunch of possible callbacks; pick one at compile-time and that's it. Simple for you (no runtime selection to debug), versatile for users.

wonder-mice commented 7 years ago

I'm thinking your "companion library" could just be an extra set of .c files that contain a bunch of possible callbacks;

Yes, I was thinking about something along this lines. I even have a local branch with skeleton for that. The basic idea is to export custom callbacks the same way zf_log exports stderr output callback. But instead of static struct instance, maybe have a function that can create one dynamically (since you need to specify a file to open, etc.). I'll post what I have today evening PST.

wonder-mice commented 7 years ago

So I was thinking about something like that basically:

/* Create log output */
zf_log_output output_open(args...);
/* Release associated resources */
void output_close(zf_log_output *output);

/* Usage example */
void foo() {
    zf_log_output out = output_open("/tmp/log");
    zf_log_set_output_p(&out);
}
fenugrec commented 7 years ago

who implements output_open() and _close() , each individual "driver"?

PS we can continue this in the "callback library" issue discussion if you prefer

wonder-mice commented 7 years ago

Makes sense, moved discussion to https://github.com/wonder-mice/zf_log/issues/9.