From 0b6c1895598f2d89c9904b3cbb36e0df30ba60f2 Mon Sep 17 00:00:00 2001 From: Adrien Destugues Date: Thu, 12 Nov 2015 10:18:35 +0100 Subject: [PATCH] BNetworkCookieJar: rework locking The cookie jar used to be locked whenever an iterator was instanciated. This didn't work well when using several iterators in the same thread, because the BLocker then allows all of them to access the list concurrently. Rework the locking code to use a more fine grained approach, where the cookie jar is only locked temporarily by methods which require it. These methods are the ones which get and put new domain-lists in the jar, as well as acquiring the locks on the domain-lists. Each domain-list in the jar is locked using a read/write lock as before. This means there can be many requests getting cookies for the same domain in paralel, but only one at a time is allowed to set new cookies. The iterators keep domain lists they need to access read-locked, as long as they iterate the cookies for that domain. A limitation of this approach is that deleting a domain-list when it becomes empty is difficult. We can live with this, however, the iteration still works (it just skips empty lists), and the empty lists will not be stored or restored when archiving the cookie jar. --- .../network/libnetapi/NetworkCookieJar.cpp | 158 +++++++++++------- 1 file changed, 98 insertions(+), 60 deletions(-) diff --git a/src/kits/network/libnetapi/NetworkCookieJar.cpp b/src/kits/network/libnetapi/NetworkCookieJar.cpp index 8c44ac906a..677cdb22a9 100644 --- a/src/kits/network/libnetapi/NetworkCookieJar.cpp +++ b/src/kits/network/libnetapi/NetworkCookieJar.cpp @@ -77,8 +77,19 @@ BNetworkCookieJar::BNetworkCookieJar(BMessage* archive) BNetworkCookieJar::~BNetworkCookieJar() { - for (Iterator it = GetIterator(); it.Next() != NULL;) + for (Iterator it = GetIterator(); it.Next() != NULL;) { delete it.Remove(); + } + + fCookieHashMap->Lock(); + + PrivateHashMap::Iterator it = fCookieHashMap->GetIterator(); + while(it.HasNext()) { + BNetworkCookieList* list = *it.NextValue(); + it.Remove(); + list->LockForWriting(); + delete list; + } delete fCookieHashMap; } @@ -95,12 +106,10 @@ BNetworkCookieJar::AddCookie(const BNetworkCookie& cookie) return B_NO_MEMORY; status_t result = AddCookie(heapCookie); - if (result != B_OK) { + if (result != B_OK) delete heapCookie; - return result; - } - return B_OK; + return result; } @@ -509,9 +518,6 @@ BNetworkCookieJar::Iterator::Iterator(const BNetworkCookieJar* cookieJar) fLastElement(NULL), fIndex(0) { - if (!fCookieJar->fCookieHashMap->Lock()) - return; - fIterator = new(std::nothrow) PrivateIterator( fCookieJar->fCookieHashMap->GetIterator()); @@ -522,15 +528,12 @@ BNetworkCookieJar::Iterator::Iterator(const BNetworkCookieJar* cookieJar) BNetworkCookieJar::Iterator::~Iterator() { - if (!fIterator) - return; - if (fList != NULL) fList->Unlock(); + if (fLastList != NULL) + fLastList->Unlock(); delete fIterator; - - fCookieJar->fCookieHashMap->Unlock(); } @@ -540,7 +543,9 @@ BNetworkCookieJar::Iterator::operator=(const Iterator& other) if (this == &other) return *this; - fCookieJar->fCookieHashMap->Unlock(); + delete fIterator; + if (fList != NULL) + fList->Unlock(); fCookieJar = other.fCookieJar; fIterator = NULL; @@ -550,13 +555,10 @@ BNetworkCookieJar::Iterator::operator=(const Iterator& other) fLastElement = NULL; fIndex = 0; - if (fCookieJar->fCookieHashMap->Lock()) - { - fIterator = new(std::nothrow) PrivateIterator( - fCookieJar->fCookieHashMap->GetIterator()); + fIterator = new(std::nothrow) PrivateIterator( + fCookieJar->fCookieHashMap->GetIterator()); - _FindNext(); - } + _FindNext(); return *this; } @@ -591,17 +593,21 @@ BNetworkCookieJar::Iterator::NextDomain() if (!fIterator->fCookieMapIterator.HasNext()) { fElement = NULL; - return NULL; + return result; } if (fList != NULL) fList->Unlock(); - fList = *fIterator->fCookieMapIterator.NextValue(); - fList->LockForReading(); + if (fCookieJar->fCookieHashMap->Lock()) { + fList = *fIterator->fCookieMapIterator.NextValue(); + fList->LockForReading(); + + fCookieJar->fCookieHashMap->Unlock(); + } + fIndex = 0; fElement = fList->ItemAt(fIndex); - return result; } @@ -615,28 +621,36 @@ BNetworkCookieJar::Iterator::Remove() const BNetworkCookie* result = fLastElement; if (fIndex == 0) { - if (fLastList->LockForWriting() == B_OK) - { - if (fLastList->CountItems() == 1) { - fIterator->fCookieMapIterator.Remove(); - delete fLastList; - fLastList = NULL; - } else { + if (fLastList && fCookieJar->fCookieHashMap->Lock()) { + // We are on the first item of fList, so we need to remove the + // last of fLastList + fLastList->Unlock(); + if (fLastList->LockForWriting() == B_OK) + { fLastList->RemoveItemAt(fLastList->CountItems() - 1); + // TODO if the list became empty, we could remove it from the + // map, but this can be a problem if other iterators are still + // referencing it. Is there a safe place and locking pattern + // where we can do that? fLastList->Unlock(); + fLastList->LockForReading(); } + fCookieJar->fCookieHashMap->Unlock(); } } else { fIndex--; - // Switch to a write lock - fList->Unlock(); - if (fList->LockForWriting() == B_OK) - { - fList->RemoveItemAt(fIndex); + if (fCookieJar->fCookieHashMap->Lock()) { + // Switch to a write lock fList->Unlock(); + if (fList->LockForWriting() == B_OK) + { + fList->RemoveItemAt(fIndex); + fList->Unlock(); + } + fList->LockForReading(); + fCookieJar->fCookieHashMap->Unlock(); } - fList->LockForReading(); } fLastElement = NULL; @@ -651,22 +665,29 @@ BNetworkCookieJar::Iterator::_FindNext() fIndex++; if (fList && fIndex < fList->CountItems()) { + // Get an element from the current list fElement = fList->ItemAt(fIndex); return; } if (fIterator == NULL || !fIterator->fCookieMapIterator.HasNext()) { + // We are done iterating fElement = NULL; return; } + // Get an element from the next list + if (fLastList != NULL) { + fLastList->Unlock(); + } fLastList = fList; - if (fList) - fList->Unlock(); + if (fCookieJar->fCookieHashMap->Lock()) { + fList = *(fIterator->fCookieMapIterator.NextValue()); + fList->LockForReading(); - fList = *(fIterator->fCookieMapIterator.NextValue()); - fList->LockForReading(); + fCookieJar->fCookieHashMap->Unlock(); + } fIndex = 0; fElement = fList->ItemAt(fIndex); @@ -711,8 +732,10 @@ BNetworkCookieJar::UrlIterator::UrlIterator(const BNetworkCookieJar* cookieJar, BNetworkCookieJar::UrlIterator::~UrlIterator() { - if (fList) + if (fList != NULL) fList->Unlock(); + if (fLastList != NULL) + fLastList->Unlock(); delete fIterator; } @@ -745,18 +768,22 @@ BNetworkCookieJar::UrlIterator::Remove() const BNetworkCookie* result = fLastElement; - if (fLastList->LockForWriting() == B_OK) - { - fLastList->RemoveItemAt(fLastIndex); - - if (fLastList->CountItems() == 0) { - HashString lastKey(fLastElement->Domain(), - fLastElement->Domain().Length()); - - delete fCookieJar->fCookieHashMap->Remove(lastKey); - } - + if (fCookieJar->fCookieHashMap->Lock()) { fLastList->Unlock(); + if (fLastList->LockForWriting() == B_OK) + { + fLastList->RemoveItemAt(fLastIndex); + + if (fLastList->CountItems() == 0) { + fIterator->fCookieMapIterator.Remove(); + delete fLastList; + fLastList = NULL; + } else { + fLastList->Unlock(); + fLastList->LockForReading(); + } + } + fCookieJar->fCookieHashMap->Unlock(); } fLastElement = NULL; @@ -841,15 +868,24 @@ BNetworkCookieJar::UrlIterator::_FindNext() { fLastIndex = fIndex; fLastElement = fElement; + if (fLastList != NULL) + fLastList->Unlock(); + fLastList = fList; + if (fLastList) + fLastList->LockForReading(); - while (!_FindPath()) { - if (!_SuperDomain()) { - fElement = NULL; - return; + if (fCookieJar->fCookieHashMap->Lock()) { + while (!_FindPath()) { + if (!_SuperDomain()) { + fElement = NULL; + fCookieJar->fCookieHashMap->Unlock(); + return; + } + + _FindDomain(); } - - _FindDomain(); + fCookieJar->fCookieHashMap->Unlock(); } } @@ -865,8 +901,9 @@ BNetworkCookieJar::UrlIterator::_FindDomain() if (fList == NULL) fElement = NULL; - else + else { fList->LockForReading(); + } fCookieJar->fCookieHashMap->Unlock(); } @@ -902,6 +939,7 @@ BNetworkCookieList::BNetworkCookieList() BNetworkCookieList::~BNetworkCookieList() { + // Note: this is expected to be called with the write lock held. pthread_rwlock_destroy(&fLock); }