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

Add memory mapped file based logging #2604

Closed
wants to merge 1 commit into from

Conversation

yuval-nextsilicon
Copy link

@yuval-nextsilicon yuval-nextsilicon commented Jan 12, 2023

Hey,

This solves a sort-of-an-issue where app crashes cause file logs to appear truncated, because the logger isn't flushed. By implementing a sink that uses memory-mapped files, we ensure that the data is still written even on abnormal exit. This allows us to enjoy a highly performant sink with very rare flushing while still getting all of our logs after a crash.

I'm not too familiar with this codebase, could you please assist me in tidying the edges of this sink (making mem_buffer_size configurable, ifdefing the entire thing only to Linux, etc.)?

@gabime
Copy link
Owner

gabime commented Jan 12, 2023

This allows us to enjoy a highly performant sink with very rare flushing while still getting all of our logs after a crash.

Thanks. So whats the down side? Is it slower than regular file sink?

@yuval-nextsilicon
Copy link
Author

This allows us to enjoy a highly performant sink with very rare flushing while still getting all of our logs after a crash.

Thanks. So whats the down side? Is it slower than regular file sink?

I don't think it is, but maybe it has slightly less optimal flushing semantics than the default FILE objects. It's also a more complicated implementation which is OS-specific (and currently only written for Linux).

@gabime
Copy link
Owner

gabime commented Jan 15, 2023

Thank, but ill have to pass on this. The preferred solution is to add some kind of fsync support for file sinks.

@gabime gabime closed this Jan 15, 2023
@yuval-nextsilicon
Copy link
Author

Thank, but ill have to pass on this. The preferred solution is to add some kind of fsync support for file sinks.

I'm... not sure how that solves the issue? fsync flushes the fd to disk, but you don't have an opportunity to do that when an app is aborted or killed. Perhaps what you mean is using posix_fadvise to prevent the kernel from flushing needlessly?

@yuval-nextsilicon
Copy link
Author

Btw, this PR solves #1607

@gabime
Copy link
Owner

gabime commented Jan 15, 2023

If a log is called and then immediately fsync is called, the chance it would crash between the 2 calls is slim. And even if this could happen, it could also crash during calling the log, and then mmap wont help.

So it is not 100% solution but it has the advantage of supporting existing sinks, providing the option to fsync only for chosen important logs, and to work for windows using FlushFileBuffers().
Of course if you need 100% solution, linux only, and limited hardcoded 2mb file, you can use a custom mmap sink.

Also, please make sure the tests pass when issuing a PR.

@neundorf
Copy link

If I add a call
setvbuf(*fp, (char *)NULL, _IOLBF, 0);
in bool fopen_s() directly after the file has been opened every line is flushed to disk, so I get a complete log in case my program crahses. Works good for me.
Could this be an option, maybe via ctor argument ?

@gabime
Copy link
Owner

gabime commented May 31, 2024

You can use the flush_on(level)function

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

Successfully merging this pull request may close these issues.

None yet

3 participants