On Fri Nov 3 18:09:59 2023 +0000, Yuxuan Shui wrote:
So, I tried, and it's difficult to validate if engine is locked or not during session shutdown, as there isn't many opportunity to hook into the engine. But that also mean it's hard for the application to observe this fact, in which case this MR still stands. And I want to argue for its inclusion.
- In general, it's a good idea to avoid nested locks. Of course, we
should avoid holding grabber locks while calling `OnProcessSample`, but: 2. That fix is less universal then this one. The problem arise from the lock order being reversed during shutdown. So there could be more potential deadlocks on top of this engine/grabber deadlock. This MR would fix all of them. I would also open an MR to do what you suggested, but I think this MR should be merged as well.
Adding to point (2), even if we can fix this now by not holding grabber lock while calling `OnProcessSample`, we might introduce more deadlocks of the same kind in the future. This MR is more future proof in that sense.