[interfacekit] Re: deadlock with BApplication/BWindow

On 2005-03-12 at 17:50:35 [+0100], Adi Oanca wrote:
> 
>     Have a poroblem when quiting be_app. I have a solution for it but I 
>     want your opinion on it.
> 
> The problem:
> ------------------------------------------
[...]
> oups, deadlock! :-)
> 
> Here's my solution:
[...]
> 
> That's it. Comments, other ideas?

In your solution you lock a window with the looper list lock being held. 
That window could just be handling a message, which might involve posting a 
message to another looper, which in turn would try to acquire the looper 
list lock. Deadlock.

Although that is not explicitely documented anywhere, I would consider the 
looper list lock an innermost lock, that is, if you hold it you should not 
try to get any other lock. This makes it save to be acquired at any point in 
the code.

A solution would hence be to limit the scope of holding the lock in 
BApplication::window_quit_loop(). The first lines would thus read:

        BList looperList;
        {
                BObjectLocker<BLooperList> listLock(gLooperList);
                if (listLock.IsLocked())
                        gLooperList.GetLooperList(&looperList);
        }

The only reason I could imagine for holding the looper list lock while 
iterating through the windows would be, that this guarantees that no new 
window is created during that time. But this doesn't solve the general race 
condition we have here: A window can just be created after the method is 
done. Putting the window_quit_loop() code into a loop, as in you proposal, 
would at least make sure that there won't be any windows anymore that have 
been created from the app thread or any window thread. So this is probably a 
good idea.

> Also, why BLooperList::GetLooperList() returns NULL(removed) loopers?
> 
> void BLooperList::GetLooperList(BList* list)
> {
>     BAutolock Listlock(fLock);
>     AssertLocked();
>     for (uint32 i = 0; i < fData.size(); ++i)
>     {
> // INSERTED by me: if (fData[i].looper)
>             list->AddItem(fData[i].looper);
>     }
> }

Yep. I never understood why Erik complicated the code by using such a sparse 
vector. The performance gain of not needing to compact the vector when a 
looper is removed should be completely neglectable for common cases (e.g. if 
you don't have hundreds of loopers and create/delete arbitrary ones 
frequently). Instead we pay a penalty when finding a looper in the vector.

CU, Ingo

Other related posts: