Logging shouldn't throw after dispose #578
Conversation
| _messageQueue.Add(message); | ||
| if (!_messageQueue.IsAddingCompleted) | ||
| { | ||
| _messageQueue.Add(message); |
There was a problem hiding this comment.
I'm not sure how often this happens in production or if it ever does, but Add blocks if the queue is full, so this could throw still.
| @@ -27,7 +27,10 @@ public ConsoleLoggerProcessor() | |||
|
|
|||
| public virtual void EnqueueMessage(LogMessageEntry message) | |||
| { | |||
| _messageQueue.Add(message); | |||
| if (!_messageQueue.IsAddingCompleted) | |||
There was a problem hiding this comment.
Other threads can call EnqueueMessage while Dispose is being called, so this doesn't get rid of the issue, just decreases how often it happens.
|
We need to fix hosting issue first, maybe this exception is not that bad too, it prevents loosing log messages. |
|
@pakrym no we don't. We need to fix both bugs. Logging is one of those things that really should never throw. People don't put try catch statements around their logs. |
|
This is it still racey as @BrennanConroy points out. I thought I read that CompleteAdding waits on any outstanding adds to finishing before switching the state but it just waits on other calls to CompleteAdding to complete. Add does wait on CompleteAdding, but then it throws. I still think this should never throw after add and this lessens the chances of that (plus this never happened before since we wrote directly to the console). I'll see if there's any other collection we could use. |
|
Will revisit |
|
I changed this PR to catch |
| var logger = new ConsoleLogger(_loggerName, filter: null, includeScopes: false, loggerProcessor: processor); | ||
| logger.Console = console; | ||
| processor.Dispose(); | ||
| logger.LogInformation("Logging after dispose"); |
There was a problem hiding this comment.
Could we change this test to check that "Logging after dispose" gets logged
- Check complete adding before adding new messages to the list. - Added a test
BrennanConroy
force-pushed
the
davidfowl/throw-after-dispose-noop
branch
from
b1ac083 to
81424d2
Compare
|
👏 |
to the list.
#563
PS: I didn't measure the performance of checking that bool on every write.