diff --git a/runtime/bin/hashmap_test.cc b/runtime/bin/hashmap_test.cc index 47849e046d4..0a4ee31ed10 100644 --- a/runtime/bin/hashmap_test.cc +++ b/runtime/bin/hashmap_test.cc @@ -171,7 +171,7 @@ void TestSet(IntKeyHash hash, int size) { } -UNIT_TEST_CASE(Set) { +UNIT_TEST_CASE(HashMap_Basic) { TestSet(WordHash, 100); TestSet(Hash, 100); TestSet(CollisionHash1, 50); @@ -180,4 +180,47 @@ UNIT_TEST_CASE(Set) { TestSet(CollisionHash4, 50); } + +UNIT_TEST_CASE(HashMap_RemoveDuringIteration) { + class Utils { + public: + static bool MatchFun(void* key1, void* key2) { return key1 == key2; } + static void* Key(intptr_t i) { return reinterpret_cast(i); } + static void* Value(intptr_t i) { return reinterpret_cast(i); } + static uint32_t HashCode(intptr_t key) { return 1; } + }; + + HashMap map(Utils::MatchFun, 8); + + // Add 6 (1, 1), ..., (6, 60) entries to the map all with a hashcode of 1 + // (i.e. have all keys have collinding hashcode). + // + // This causes the probing position in the hashmap to be 1 and open-addressing + // with linear probing will fill in the slots towards the right + // (i.e. from 1..6). + for (intptr_t i = 1; i <= 6 /* avoid rehash at 7 */; i++) { + HashMap::Entry* entry = map.Lookup(Utils::Key(i), Utils::HashCode(i), true); + entry->value = Utils::Value(10 * i); + } + + // Now we iterate over all elements and delete the current element. Since all + // our entries have a colliding hashcode of 1, each deletion will cause all + // following elements to be left-rotated by 1. + intptr_t i = 0; + HashMap::Entry* current = map.Start(); + while (current != NULL) { + i++; + EXPECT_EQ(Utils::Key(i), current->key); + EXPECT_EQ(Utils::Value(10 * i), current->value); + + // Every 2nd element we keep to hit the left-rotation case only sometimes. + if (i % 2 == 0) { + current = map.Remove(current); + } else { + current = map.Next(current); + } + } + EXPECT_EQ(6, i); +} + } // namespace dart diff --git a/runtime/bin/socket.cc b/runtime/bin/socket.cc index d5205723cc8..cbb9a2c937d 100644 --- a/runtime/bin/socket.cc +++ b/runtime/bin/socket.cc @@ -196,7 +196,8 @@ Dart_Handle ListeningSocketRegistry::CreateBindListen(Dart_Handle socket_object, } -bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket) { +bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket, + bool update_hash_maps) { ASSERT(!mutex_->TryLock()); ASSERT(os_socket != NULL); ASSERT(os_socket->ref_count > 0); @@ -204,26 +205,28 @@ bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket) { if (os_socket->ref_count > 0) { return false; } - // We free the OS socket by removing it from two datastructures. - RemoveByFd(os_socket->socketfd); + if (update_hash_maps) { + // We free the OS socket by removing it from two datastructures. + RemoveByFd(os_socket->socketfd); - OSSocket* prev = NULL; - OSSocket* current = LookupByPort(os_socket->port); - while (current != os_socket) { - ASSERT(current != NULL); - prev = current; - current = current->next; - } + OSSocket* prev = NULL; + OSSocket* current = LookupByPort(os_socket->port); + while (current != os_socket) { + ASSERT(current != NULL); + prev = current; + current = current->next; + } - if ((prev == NULL) && (current->next == NULL)) { - // Remove last element from the list. - RemoveByPort(os_socket->port); - } else if (prev == NULL) { - // Remove first element of the list. - InsertByPort(os_socket->port, current->next); - } else { - // Remove element from the list which is not the first one. - prev->next = os_socket->next; + if ((prev == NULL) && (current->next == NULL)) { + // Remove last element from the list. + RemoveByPort(os_socket->port); + } else if (prev == NULL) { + // Remove first element of the list. + InsertByPort(os_socket->port, current->next); + } else { + // Remove element from the list which is not the first one. + prev->next = os_socket->next; + } } ASSERT(os_socket->ref_count == 0); @@ -234,9 +237,10 @@ bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket) { void ListeningSocketRegistry::CloseAllSafe() { MutexLocker ml(mutex_); - for (HashMap::Entry* p = sockets_by_fd_.Start(); p != NULL; - p = sockets_by_fd_.Next(p)) { - CloseOneSafe(reinterpret_cast(p->value)); + + for (HashMap::Entry* cursor = sockets_by_fd_.Start(); cursor != NULL; + cursor = sockets_by_fd_.Next(cursor)) { + CloseOneSafe(reinterpret_cast(cursor->value), false); } } @@ -245,7 +249,7 @@ bool ListeningSocketRegistry::CloseSafe(intptr_t socketfd) { ASSERT(!mutex_->TryLock()); OSSocket* os_socket = LookupByFd(socketfd); if (os_socket != NULL) { - return CloseOneSafe(os_socket); + return CloseOneSafe(os_socket, true); } else { // It should be impossible for the event handler to close something that // hasn't been created before. diff --git a/runtime/bin/socket.h b/runtime/bin/socket.h index 35d4acc870f..1afc05e58a3 100644 --- a/runtime/bin/socket.h +++ b/runtime/bin/socket.h @@ -469,7 +469,7 @@ class ListeningSocketRegistry { void InsertByFd(intptr_t fd, OSSocket* socket); void RemoveByFd(intptr_t fd); - bool CloseOneSafe(OSSocket* os_socket); + bool CloseOneSafe(OSSocket* os_socket, bool update_hash_maps); void CloseAllSafe(); HashMap sockets_by_port_; diff --git a/runtime/platform/hashmap.cc b/runtime/platform/hashmap.cc index 46115339812..4d4b4774dd3 100644 --- a/runtime/platform/hashmap.cc +++ b/runtime/platform/hashmap.cc @@ -106,11 +106,27 @@ void HashMap::Remove(void* key, uint32_t hash) { // Clear the candidate which will not break searching the hash table. candidate->key = NULL; - candidate->value = NULL; occupancy_--; } +HashMap::Entry* HashMap::Remove(Entry* entry) { + Remove(entry->key, entry->hash); + + // A key can only exist once in the map and we just removed `key`. This means + // that either a left-rotation has happened (in which case `entry` points + // already to the next element (in terms of iteration order)) or alternatively + // we can use the normal `Next()` call to move in iteration order. + if (entry->key != NULL) { + // A left-rotation happened. `entry` points already to the next element in + // iteration order. + return entry; + } else { + return Next(entry); + } +} + + void HashMap::Clear(ClearFun clear) { // Mark all entries as empty. const Entry* end = map_end(); @@ -118,7 +134,6 @@ void HashMap::Clear(ClearFun clear) { if ((clear != NULL) && (p->key != NULL)) { clear(p->value); } - p->value = NULL; p->key = NULL; } occupancy_ = 0; diff --git a/runtime/platform/hashmap.h b/runtime/platform/hashmap.h index 1fd1235153d..240e94d1a0f 100644 --- a/runtime/platform/hashmap.h +++ b/runtime/platform/hashmap.h @@ -61,8 +61,28 @@ class HashMap { Entry* Lookup(void* key, uint32_t hash, bool insert); // Removes the entry with matching key. + // + // WARNING: This method cannot be called while iterating a `HashMap` + // otherwise the iteration might step over elements! void Remove(void* key, uint32_t hash); + // Removes the entry and returns the next entry (in iteration order) after + // this one. + // + // Usage instructions for removing elements during iteration: + // + // HashMap::Entry* current = map.Start(); + // while (current != NULL) { + // ... + // if (condition) { + // current = map.Remove(current); + // } else { + // current = map.Next(current); + // } + // } + // + Entry* Remove(Entry* entry); + // Empties the hash map (occupancy() == 0), and calls the function 'clear' on // each of the values if given. void Clear(ClearFun clear = NULL);