Fix deadlock in BuildManager vs LoggingService #6837
Conversation
| // in the work queue, but the top level exception handler there should catch everything and have forwarded it to the | ||
| // OnThreadException method in this class already. | ||
| _workQueue.Complete(); | ||
| Task.WaitAny(_workQueue.Completion); |
There was a problem hiding this comment.
Why WaitAny when you're passing only one task?
There was a problem hiding this comment.
Because WaitAny is only Task.WaitXX I am aware of which does not throw Exception if awaited task ends up in a Failed state. Exceptions during _workQueue DataFlow processing are handled in ProcessWorkQueue the only exceptions left this could throw, AFAIK, is OperationCanceledException which we shall swallow anyway.
There was a problem hiding this comment.
Interesting! And very unobvious. Would an explicit try/catch with a comment be more appropriate? Or at least a comment.
(Perf super nit: WaitAny takes params Task[] so the call allocates an array.)
There was a problem hiding this comment.
at least a comment
Yes, please.
There was a problem hiding this comment.
I have though about it more, and it looks like it shall never throws. It will be better if that unexpected exception will surface by _workQueue.Completion.Wait();.
| // in the work queue, but the top level exception handler there should catch everything and have forwarded it to the | ||
| // OnThreadException method in this class already. | ||
| _workQueue.Complete(); | ||
| Task.WaitAny(_workQueue.Completion); |
There was a problem hiding this comment.
at least a comment
Yes, please.
Fixes #6823
Context
There is possible deadlock in BuildManager vs LoggingService when:
LoggerMode.SynchronousChanges Made
LoggingService callbacks (OnProjectStarted, OnProjectFinished and OnThreadException) has been modified into async by leveraging existing workQueue.
Testing
Compiled Orchardcore in CLI and VS2022.
Notes
If these changes are considered safe, we can optionally revert PR #6717