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.
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
Introduce a non blocking file appender #673
Introduce a non blocking file appender #673
Changes from 18 commits
945f4a8
9ccaa5d
2a34863
39f2709
f251bbe
de77617
7c9ce25
e028dfa
55bdffe
1a70543
5b4eacf
18d0340
c4a8447
1c33af9
6e8cb23
75ee728
e5944c0
cc4fde7
d9968b2
a1f0d01
9316656
ac14ca3
b3aadef
f77a6a8
4417342
64b356f
98db6f7
ec049e0
be7aeb4
e87b631
5d8475c
2983370
8e6a395
80bf8e2
8f9bdc1
e467bdd
4b73a1c
2b6cb51
9e7620d
b415265
4ed792c
93b26af
f494d53
5216e6c
97e5e15
714e5bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, it looks like this automatically spawns the worker thread? I wonder if we should also have an API that returns a
Worker
struct with a publicWorker::run
method (or something) that runs the worker main loop. That way, the user can spawn the worker thread manually, allowing them to control things like the thread's name and how panics are propagated. We could continue to provide an API that spawns the worker thread as well, since it's probably more ergonomic when the user doesn't need control over spawning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, calling build will result in spawning of the worker thread.
I'll implement your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do the same by e.g. passing or parameterizing over a
ThreadFactory
, which is the common thing that is done in Java.I am not convinced users should by required to manage the lifetime of threads - that just leaks implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Matthias247 that's a good point, we could have a builder method that takes a
std::thread::Builder
, but still handles spawning the thread and holding onto theJoinHandle
internally.I'm not sure if there are still valid use-cases for manually running the worker thread, if we can customize the name and stack size. Perhaps some users might want to run the logging work on an existing thread that has already finished some other task (e.g. on the main thread if the rest of the application runs on a thread pool)? But, that seems niche enough that we can always add support for it if anyone actually asks for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think being able to run the work itself is good if the work is rather short-lived, and you can e.g. schedule it on an existing threadpool. However an appender like this is typically very long lived - you create it once and it runs for the lifetime of the application. That's not something you would schedule on a threadpool. Therefore I agree that other use-case seem niche.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allocating a vector for each line will create a lot of memory churn, and I expect it to noticeably show up in profiles if logging is enabled. An alternative solution could be to have a pool of logging buffers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Matthias247 I've started working on a solution for that separately (a ring buffer of strings, basically), and I'm hoping we can swap that in as a replacement for
crossbeam_channel
in the future. For this PR, I want to focus on the API design.