Mir

Code review comment for lp:~mir-team/mir/fix-1339700

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> Alberto: framedrop_policy is a member of BufferQueue and as such is protected
> by BufferQueue.guard. Giving framedrop_policy its own internal locking and
> making it an exceptional member I guess could be safe (maybe?), but it's
> better if we don't force anyone to have to think that hard. Because tricky
> exceptions like that just result in future misunderstandings and bugs.
>

Mutual exclusion protection is only needed if:
1) The member variable itself is mutating - it's not in this case even though it's not const.
2) The methods called on the object are not thread safe - from what I can see the implementation (TimeoutFrameDroppingPolicy) seems to be thread safe with respect to the two methods we invoke (swap_now_blocking and swap_unblocked)

I agree, we need to fix the root of the issue - but I think this is a viable short term bandaid.

> I've proposed a simpler fix here with a test (which successfully detects the
> deadlock without hanging itself):
> https://code.launchpad.net/~vanvugt/mir/fix-1339700-alarm/+merge/226252

I explicitly avoided dropping the lock at the AlarmImpl since it brings up lifetime issues which Alan addressed.

« Back to merge proposal