-
Notifications
You must be signed in to change notification settings - Fork 13
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
Flush AsyncPoolSink
on the current thread if tearing down
#66
Conversation
15c674b
to
9d7dafd
Compare
AsyncPoolSink
on the current thread if tearing down, drop the default logger in atexit
callback
e5cc64d
to
e1f9a06
Compare
Benchmark resultsOn ArchLinux, commit e1f9a06 Before
After
Guess the main reason for the worse is the new |
e1f9a06
to
ecdc8e0
Compare
If we use
|
ecdc8e0
to
b6715fd
Compare
c241392
to
1a4b06d
Compare
Reverted commit |
AsyncPoolSink
on the current thread if tearing down, drop the default logger in atexit
callbackAsyncPoolSink
on the current thread if tearing down
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.
LGTM.
Fixes #64.
I've figured out what happened with this bug. There are 2 things went wrong:
crossbeam
uses thread-local internally, which is not supported inatexit
callback. This is the reason of panicuse of std::thread::current() is not possible after the thread's local data has been destroyed
.Fixed it by directly flushing the sinks on the current thread if the program is tearing down.
The default logger just gets flushed in
atexit
callback, but not dropped. This results in the program not waiting for the asynchronous thread to finish but simply exiting after the task has been sent.Fixed it by changing the global variable withReverted, paragraph 3 addresses the problem.Option<T>
, so that we could explicitly drop the default logger inatexit
callback.UPDATE: Destroying the thread pool before the final flush is required as well, otherwise pending log tasks may be lost.
Fixing this by
adding aand explicitly destroy the thread pool before the final flush, but needs to be benchmarked to see if the performance change is acceptable.RwLock
toThreadPool
UPDATE: Changed to
ArcSwapOption
for a better performance.Related discussion #63 #65, CC @MrCoco5921 @Bestfast