-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 ConsoleLogger option - or expose a synchronization primitive - so ConsoleLogger plays nice with interactive Console applications #52807
Comments
Tagging subscribers to this area: @carlossanlop Issue DetailsBackground and MotivationThe
Proposed API(Unrealistic) Solution: Make
|
Author: | Jehoel |
---|---|
Assignees: | - |
Labels: |
|
Milestone: | - |
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue DetailsBackground and MotivationThe
Proposed API(Unrealistic) Solution: Make
|
Author: | daiplusplus |
---|---|
Assignees: | - |
Labels: |
|
Milestone: | Future |
Background and Motivation
The
Microsoft.Extensions.Logging.Console
'sConsoleLoggerProcessor
class iterates overBlockingCollection
and writes formatted messages to the console in a new, separate, and private thread. There are 3 problems with the current design when used in a console application program:Console
directly while also calling-into services that log messages then colored message text will clash with the application's writes.Console.WriteLine(String)
will not be intermingled (i.e. they'll appear as two separate uncorrupted lines), if one thread needs to render multiple lines together then that cannot be guaranteed as theConsoleLoggerProcessor
thread may have itsConsole.Write
calls run in-between the other thread's calls.System.Console
.Main
without disposing of the DI container.Proposed API
(Unrealistic) Solution: Make
IConsole
a public DI service and add methods to allow consumers to temporarily borrow exclusive access:As much as we'd all like
System.Console
to go-away and be replaced with an injectableIConsole
interface, that's too much to ask for - and even if we keptSystem.Console
and merely addedIConsole
as an improved way for working with the console/terminal/stdout it would be subject to too much bikeshedding so I won't go into detail on this option....but if we did, I'd want it to have something like this:
(Realistic) Solution 2: Allow a
lock()
-able object to be specified inConsoleLoggerOptions
which is respected byConsoleLoggerProcessor
.ConsoleLoggerOptions.ConsoleLockObject
is not-null, thenConsoleLoggerProcessor.WriteMessage
method would lock onConsoleLoggerOptions.ConsoleLockObject
(if specified) so that each message can be written atomically and not when application code is locked on the same object to gain exclusive use ofConsole
for an uninterruptable sequence of writes.ProcessLogQueue()
to lock onConsoleLockObject
for the entire batch of pending messages, so all pending messages are written without any interleaving writes from application code (assuming that application code is also locking on the sameConsoleLockObject
of course).BlockingCollection<T>
- while I see thatConsoleLoggerProcessor
uses a singleforeach
withGetConsumingEnumerable
, I assume it's possible to detect when there's a batch of pending messages and to use a single lock for all of them, instead of individually.Note that while consuming applications would be free to use
lock()
to synchronize their ownConsole
use-sites, theConsoleLoggerProcessor
would always useMonitor.TryEnter
with theConsoleLockTimeout
to prevent deadlocks.An example of its usage:
Solution addendum for pending console log messages at the end of
Main
: Add a code-analysis checkerA simple solution for the problem of trivial
Main
methods returning (and causing an entire application shutdown) beforeConsoleLoggerProcessor
has finished writing all pending messages to the console would be the introduction of a Roslyn code-analyzer that warns the user if anIHost
is created insideMain
but not disposed beforeMain
returns.So the code above (and the linked example) would trigger an analysis warning that
Main
does not dispose ofIHost
and/or callStopAsync
before returning.Triggering code example:
Code-fix result example:
The code-analysis code-fix would be to wrap the
IHost
in an explicitusing
block - but exclude any trailing use ofConsole
:Risks
As with any use of
lock
, the opportunity for deadlocks is present, however because only the user's application code would ever uselock()
and the library code will only ever useMonitor.TryEnter
there shouldn't be any issues there.Because C# already disallows the use of
await
inside alock
I don't forsee there being any major issues.Contribution / PR
As per the Contribution guidelines and API review process, because this is a public API addition, I won't submit my PR with my changes (and tests) until this suggestion is accepted.
The text was updated successfully, but these errors were encountered: