[interfacekit] Re: deadlock with BApplication/BWindow

Hi,


Then again I fail to see the use of these lines in _Lock():
-------------------------
                BObjectLocker<BLooperList> ListLock(gLooperList);
                if (!ListLock.IsLocked())
                {
                        // If we can't lock, the semaphore is probably
                        // gone, which leaves us in no-man's land
PRINT(("BLooper::_Lock() done 2\n"));
                        return B_BAD_VALUE;
                }
        
                //      Look up looper by port_id, if necessary
                if (!loop)
                {
                        loop = LooperForPort(port);
                        if (!loop)
                        {
PRINT(("BLooper::_Lock() done 3\n"));
                                return B_BAD_VALUE;
                        }
                }
                else
                {
                        //      Check looper validity
                        if (!IsLooperValid(loop))
                        {
PRINT(("BLooper::_Lock() done 4\n"));
                                return B_BAD_VALUE;
                        }
                }
---------------------------

        _Lock(BLooper* loop, port_id port, bigtime_t timeout)
is always called with 'this' and '-1'
so 'loop' will always be valid. IsLooperValid() checks for 'loop' in the global
list and if not, return error. But this will always return true, unless the 
looper
has been deleted and this is an illegal call, in which case it's right to fail 
with
a segmentation fault.

        Maybe I am missing something, and if that's the case, please tell me 
what.
        If not, can we please remove the above lines? They are useless and 
introduce
lots of CPU cycles.


Thanks,
Adi.


Adi Oanca wrote: 
Hi guys,


Have a poroblem when quiting be_app. I have a solution for it but I want your opinion on it.

The problem:
------------------------------------------
bool
BApplication::QuitRequested()
{
    return quit_all_windows(false);
}
bool
BApplication::quit_all_windows(bool force)
{
    AssertLocked();

    if (window_quit_loop(false, force))
        return true;

    return window_quit_loop(true, force);
}
bool
BApplication::window_quit_loop(bool quitFilePanels, bool force)
{
    BList looperList;
 >>>>>>>>BObjectLocker<BLooperList> listLock(gLooperList);
    if (listLock.IsLocked())
        gLooperList.GetLooperList(&looperList);

for (int32 i = looperList.CountItems(); i-- > 0; ) {
BWindow *window = dynamic_cast<BWindow *>((BLooper *)looperList.ItemAt(i));

// ToDo: windows in this list may already have been closed in the mean time?!

        if (window != NULL && window->Lock()) {
            if ((window->IsFilePanel() && !quitFilePanels)
                || (!force && !window->QuitRequested())) {
                // the window does not want to quit, so we don't either
                window->Unlock();
                return false;
            }

 >>>>>>>>>>>        window->Quit();
        }
    }

    return true;
}

void BWindow::Quit(){

...
BLooper::Quit();
}
void BLooper::Quit()
{
else
{
PRINT((" Run() has already been called and we are not the looper thread\n"));

        BMessage message(_QUIT_);
        message.AddInt32("testfield", 42);
        err = PostMessage(&message);

        // Also as per the BeBook, we have to wait until the looper is done
        // processing any remaining messages.
        int32 temp;
        do
        {
PRINT(("  wait_for_thread(%ld)...\n", tid));
            err = wait_for_thread(tid, &temp);
        } while (err == B_INTERRUPTED);
    }
}
------------------------------------------

in BWindow's task_looper() after _QUIT_ is received, Lock() is called:

------------------------------------------
bool BLooper::Lock()
{
    // Defer to global _Lock(); see notes there
    return _Lock(this, -1, B_INFINITE_TIMEOUT) == B_OK;
}
status_t BLooper::_Lock(BLooper* loop, port_id port, bigtime_t timeout)
{
...
/**
    @note    We lock the looper list at the start of the lock operation to
            prevent the looper getting removed from the list while we're
            doing list operations.  Also ensures that the looper doesn't
            get deleted here (since ~BLooper() has to lock the list as
            well to remove itself).
 */
    {
 >>>>>>>>>>>>    BObjectLocker<BLooperList> ListLock(gLooperList);
...

        // sLooperListLock automatically released here
    }
...
}
------------------------------------------

oups, deadlock! :-)

Here's my solution:

BApplication::window_quit_loop(bool quitFilePanels, bool force)
{
    BList looperList;
    int32 i;
    BWindow *window;
    bool moreWindows = true;

    while (moreWindows)
    {
        gLooperList.Lock();
        if (gLooperList.IsLocked())
        {
            moreWindows = false;

looperList.MakeEmpty();
gLooperList.GetLooperList(&looperList);
for (i = looperList.CountItems(); i-- > 0; )
{
window = dynamic_cast<BWindow *>((BLooper *)looperList.ItemAt(i));
if (window != NULL && window->Lock()) {
moreWindows = true;
if ((window->IsFilePanel() && !quitFilePanels)
|| (!force && !window->QuitRequested())) {
// the window does not want to quit, so we don't either
window->Unlock();
return false;
}

                    // 'window' lock is acquired.
                    // TODO: still, is this safe? can 'window' be invalid?
                    gLooperList.Unlock();

                    window->Quit();

                    // go for 'while'.
                    // a window in current list may already be invalid.
                    break;
                }
            }
            if (!moreWindows)
                gLooperList.Unlock();
        }
        else
            return false;
    }

    return true;
}

That's it. Comments, other ideas?


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);
    }
}


Thanks,
Adi.




Other related posts: