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
Enabling console logger queue length and mode to be configurable #70186
Enabling console logger queue length and mode to be configurable #70186
Changes from 16 commits
3a45e15
0da2fd1
ace65f0
00d3e8d
1ae1440
e963406
99601ed
49a27e0
c559455
a4f9176
b55de82
a3d550a
60f5f3b
f200c8c
f74dd41
b4458be
5ded943
10fbf54
263e5f3
a369c54
04d9fb5
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.
Why don't we need a
Monitor.PulseAll(_messageQueue);
here?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.
no thread would be waiting on full mode to change
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.
what about my comment here #70186 (comment)?
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.
oh seems like this comment is related, #70186 (comment)
updated.
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.
So after the
Dispose
is called - we won't log any messages that are still in buffer?@vlada-shubina as FYI
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.
To elaborate more
Dispose
callsCompleteAdding
which tries to lock_messageQueue
and then marks_isAddingCompleted
withtrue
ProcessLogQueue
callsTryDequeue
in a loop, per single item. For each call it needs to lock_messageQueue
to be able to return item to be logged byProcessLogQueue
_isAddingCompleted
istrue
theTryDequeue
will return false - even if_messageQueue.Count
is nonemptyThose facts bring the following concerns:
CompleteAdding
andTryDequeue
races for same lock. OnceCompleteAdding
wins, nothing more is logged, even if items are still queued - this seems to be a bug.TryDequeue
tries to acquire lock for each single item being logged - I'm surprised this ends up being faster then using low-lockBlockingCollection
. If it really is - should theBlockingCollection
be actualy revised?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.
@JanKrivanek if you can provide a sample code to demonstrate the concerns, you may open a new issue and we can track looking at that there. Thanks for your feedback.
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 am seeing you already logged #79812. We'll try to look at that at some point.
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.
Sample repro: https://github.com/JanKrivanek/ConsoleLogging
Showing that messages are being dropped in 7.0 version of logging.