Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After a static code analysis with Coverity last week, and the issue opened in #3427, we decided to try and refactor the way logging is performed internally, which is what this PR does.
Rather than using growing and reusable buffers as before, we switched to an async queue with smaller allocations for each item passed to the logger. While this increases the number of allocations, it's much more thread safe than before: fixing the broken behaviour the old code could have (and in practice pretty much never had, except for rare occurrences like the issue above) would have meant expanding the scope of the existing mutexes, which could have risked bottlenecks in potentially critical parts of the code (we definitely don't want queueing something for the logs to wait).
I took advantage of this refactoring to also fix another sometimes inconsistent behaviour, that is some initial lines that would always be missing when using log files or external loggers. The new queueing mechanism now takes that into account.
We performed some stress testing and comparisons, and apparently the new approach is slightly faster in the tests we made. Please do test the PR, especially if you have "chatty" Janus instances, and let us know if you encounter any issues before we merge this. It's a PR for
master
, at the moment, but I'll backport it to0.x
too when done.