On Wed Nov 1 13:47:13 2023 +0000, Yuxuan Shui wrote:
oh, ok. i can write a test to check if session shutdown is called with engine locked.
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.
1. 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.