fix for infinite loop in erase entities special packet

assignment-client/src/entities/EntityServer.cpp

@@ -101,24 +101,121 @@ int EntityServer::sendSpecialPackets(const SharedNodePointer& node, OctreeQueryN
 
     EntityNodeData* nodeData = static_cast<EntityNodeData*>(node->getLinkedData());
     if (nodeData) {
+
         quint64 deletedEntitiesSentAt = nodeData->getLastDeletedEntitiesSentAt();
+        quint64 considerEntitiesSince = EntityTree::getAdjustedConsiderSince(deletedEntitiesSentAt);
+
         quint64 deletePacketSentAt = usecTimestampNow();
         EntityTreePointer tree = std::static_pointer_cast<EntityTree>(_tree);
+        auto recentlyDeleted = tree->getRecentlyDeletedEntityIDs();
         bool hasMoreToSend = true;
 
         packetsSent = 0;
 
-        while (hasMoreToSend) {
+        // create a new special packet
-            auto specialPacket = tree->encodeEntitiesDeletedSince(queryNode->getSequenceNumber(), deletedEntitiesSentAt,
+        std::unique_ptr<NLPacket> deletesPacket = NLPacket::create(PacketType::EntityErase);
-                                                                  hasMoreToSend);
 
-            queryNode->packetSent(*specialPacket);
+        // pack in flags
+        OCTREE_PACKET_FLAGS flags = 0;
+        deletesPacket->writePrimitive(flags);
 
-            totalBytes += specialPacket->getDataSize();
+        // pack in sequence number
-            packetsSent++;
+        auto sequenceNumber = queryNode->getSequenceNumber();
+        deletesPacket->writePrimitive(sequenceNumber);
 
-            DependencyManager::get<NodeList>()->sendPacket(std::move(specialPacket), *node);
+        // pack in timestamp
-        }
+        OCTREE_PACKET_SENT_TIME now = usecTimestampNow();
+        deletesPacket->writePrimitive(now);
+
+        // figure out where we are now and pack a temporary number of IDs
+        uint16_t numberOfIDs = 0;
+        qint64 numberOfIDsPos = deletesPacket->pos();
+        deletesPacket->writePrimitive(numberOfIDs);
+
+        // we keep a multi map of entity IDs to timestamps, we only want to include the entity IDs that have been
+        // deleted since we last sent to this node
+        auto it = recentlyDeleted.constBegin();
+        while (it != recentlyDeleted.constEnd()) {
+
+            // if the timestamp is more recent then out last sent time, include it
+            if (it.key() > considerEntitiesSince) {
+
+                // get all the IDs for this timestamp
+                const auto& entityIDsFromTime = recentlyDeleted.values(it.key());
+
+                for (const auto& entityID : entityIDsFromTime) {
+
+                    // check to make sure we have room for one more ID, if we don't have more
+                    // room, then send out this packet and create another one
+                    if (NUM_BYTES_RFC4122_UUID > deletesPacket->bytesAvailableForWrite()) {
+
+                        // replace the count for the number of included IDs
+                        deletesPacket->seek(numberOfIDsPos);
+                        deletesPacket->writePrimitive(numberOfIDs);
+
+                        // Send the current packet
+                        queryNode->packetSent(*deletesPacket);
+                        auto thisPacketSize = deletesPacket->getDataSize();
+                        totalBytes += thisPacketSize;
+                        packetsSent++;
+                        DependencyManager::get<NodeList>()->sendPacket(std::move(deletesPacket), *node);
+
+                        #ifdef EXTRA_ERASE_DEBUGGING
+                            qDebug() << "EntityServer::sendSpecialPackets() sending packet packetsSent[" << packetsSent << "] size:" << thisPacketSize;
+                        #endif
+
+
+                        // create another packet
+                        deletesPacket = NLPacket::create(PacketType::EntityErase);
+
+                        // pack in flags
+                        deletesPacket->writePrimitive(flags);
+
+                        // pack in sequence number
+                        sequenceNumber = queryNode->getSequenceNumber();
+                        deletesPacket->writePrimitive(sequenceNumber);
+
+                        // pack in timestamp
+                        deletesPacket->writePrimitive(now);
+
+                        // figure out where we are now and pack a temporary number of IDs
+                        numberOfIDs = 0;
+                        numberOfIDsPos = deletesPacket->pos();
+                        deletesPacket->writePrimitive(numberOfIDs);
+                    }
+
+                    // FIXME - we still seem to see cases where incorrect EntityIDs get sent from the server
+                    // to the client. These were causing "lost" entities like flashlights and laser pointers
+                    // now that we keep around some additional history of the erased entities and resend that
+                    // history for a longer time window, these entities are not "lost". But we haven't yet
+                    // found/fixed the underlying issue that caused bad UUIDs to be sent to some users.
+                    deletesPacket->write(entityID.toRfc4122());
+                    ++numberOfIDs;
+
+                    #ifdef EXTRA_ERASE_DEBUGGING
+                        qDebug() << "EntityTree::encodeEntitiesDeletedSince() including:" << entityID;
+                    #endif
+                } // end for (ids)
+
+            } // end if (it.val > sinceLast)
+
+
+            ++it;
+        } // end while
+
+        // replace the count for the number of included IDs
+        deletesPacket->seek(numberOfIDsPos);
+        deletesPacket->writePrimitive(numberOfIDs);
+
+        // Send the current packet
+        queryNode->packetSent(*deletesPacket);
+        auto thisPacketSize = deletesPacket->getDataSize();
+        totalBytes += thisPacketSize;
+        packetsSent++;
+        DependencyManager::get<NodeList>()->sendPacket(std::move(deletesPacket), *node);
+        #ifdef EXTRA_ERASE_DEBUGGING
+            qDebug() << "EntityServer::sendSpecialPackets() sending packet packetsSent[" << packetsSent << "] size:" << thisPacketSize;
+        #endif
 
         nodeData->setLastDeletedEntitiesSentAt(deletePacketSentAt);
     }
@@ -134,6 +231,7 @@ int EntityServer::sendSpecialPackets(const SharedNodePointer& node, OctreeQueryN
     return totalBytes;
 }
 
+
 void EntityServer::pruneDeletedEntities() {
     EntityTreePointer tree = std::static_pointer_cast<EntityTree>(_tree);
     if (tree->hasAnyDeletedEntities()) {

libraries/entities/src/EntityTree.cpp

@@ -801,8 +801,13 @@ void EntityTree::update() {
     }
 }
 
+quint64 EntityTree::getAdjustedConsiderSince(quint64 sinceTime) {
+    return (sinceTime - DELETED_ENTITIES_EXTRA_USECS_TO_CONSIDER);
+}
+
+
 bool EntityTree::hasEntitiesDeletedSince(quint64 sinceTime) {
-    quint64 considerEntitiesSince = sinceTime - DELETED_ENTITIES_EXTRA_USECS_TO_CONSIDER;
+    quint64 considerEntitiesSince = getAdjustedConsiderSince(sinceTime);
 
     // we can probably leverage the ordered nature of QMultiMap to do this quickly...
     bool hasSomethingNewer = false;
@@ -829,88 +834,6 @@ bool EntityTree::hasEntitiesDeletedSince(quint64 sinceTime) {
     return hasSomethingNewer;
 }
 
-// sinceTime is an in/out parameter - it will be side effected with the last time sent out
-std::unique_ptr<NLPacket> EntityTree::encodeEntitiesDeletedSince(OCTREE_PACKET_SEQUENCE sequenceNumber, quint64& sinceTime,
-                                                                 bool& hasMore) {
-    quint64 considerEntitiesSince = sinceTime - DELETED_ENTITIES_EXTRA_USECS_TO_CONSIDER;
-    auto deletesPacket = NLPacket::create(PacketType::EntityErase);
-
-    // pack in flags
-    OCTREE_PACKET_FLAGS flags = 0;
-    deletesPacket->writePrimitive(flags);
-
-    // pack in sequence number
-    deletesPacket->writePrimitive(sequenceNumber);
-
-    // pack in timestamp
-    OCTREE_PACKET_SENT_TIME now = usecTimestampNow();
-    deletesPacket->writePrimitive(now);
-
-    // figure out where we are now and pack a temporary number of IDs
-    uint16_t numberOfIDs = 0;
-    qint64 numberOfIDsPos = deletesPacket->pos();
-    deletesPacket->writePrimitive(numberOfIDs);
-
-    // we keep a multi map of entity IDs to timestamps, we only want to include the entity IDs that have been
-    // deleted since we last sent to this node
-    {
-        QReadLocker locker(&_recentlyDeletedEntitiesLock);
-
-        bool hasFilledPacket = false;
-
-        auto it = _recentlyDeletedEntityItemIDs.constBegin();
-        while (it != _recentlyDeletedEntityItemIDs.constEnd()) {
-            QList<QUuid> values = _recentlyDeletedEntityItemIDs.values(it.key());
-            for (int valueItem = 0; valueItem < values.size(); ++valueItem) {
-
-                // if the timestamp is more recent then out last sent time, include it
-                if (it.key() > considerEntitiesSince) {
-                    QUuid entityID = values.at(valueItem);
-
-                    // FIXME - we still seem to see cases where incorrect EntityIDs get sent from the server
-                    // to the client. These were causing "lost" entities like flashlights and laser pointers
-                    // now that we keep around some additional history of the erased entities and resend that
-                    // history for a longer time window, these entities are not "lost". But we haven't yet
-                    // found/fixed the underlying issue that caused bad UUIDs to be sent to some users.
-                    deletesPacket->write(entityID.toRfc4122());
-                    ++numberOfIDs;
-
-                    #ifdef EXTRA_ERASE_DEBUGGING
-                        qDebug() << "EntityTree::encodeEntitiesDeletedSince() including:" << entityID;
-                    #endif
-
-                    // check to make sure we have room for one more ID
-                    if (NUM_BYTES_RFC4122_UUID > deletesPacket->bytesAvailableForWrite()) {
-                        hasFilledPacket = true;
-                        break;
-                    }
-                }
-            }
-
-            // check to see if we're about to return
-            if (hasFilledPacket) {
-                // let our caller know how far we got
-                sinceTime = it.key();
-                break;
-            }
-
-            ++it;
-        }
-
-        // if we got to the end, then we're done sending
-        if (it == _recentlyDeletedEntityItemIDs.constEnd()) {
-            hasMore = false;
-        }
-    }
-
-    // replace the count for the number of included IDs
-    deletesPacket->seek(numberOfIDsPos);
-    deletesPacket->writePrimitive(numberOfIDs);
-
-    return deletesPacket;
-}
-
-
 // called by the server when it knows all nodes have been sent deleted packets
 void EntityTree::forgetEntitiesDeletedBefore(quint64 sinceTime) {
     quint64 considerSinceTime = sinceTime - DELETED_ENTITIES_EXTRA_USECS_TO_CONSIDER;

libraries/entities/src/EntityTree.h

@@ -147,10 +147,19 @@ public:
     void addNewlyCreatedHook(NewlyCreatedEntityHook* hook);
     void removeNewlyCreatedHook(NewlyCreatedEntityHook* hook);
 
-    bool hasAnyDeletedEntities() const { return _recentlyDeletedEntityItemIDs.size() > 0; }
+    bool hasAnyDeletedEntities() const { 
+        QReadLocker locker(&_recentlyDeletedEntitiesLock);
+        return _recentlyDeletedEntityItemIDs.size() > 0;
+    }
+
     bool hasEntitiesDeletedSince(quint64 sinceTime);
-    std::unique_ptr<NLPacket> encodeEntitiesDeletedSince(OCTREE_PACKET_SEQUENCE sequenceNumber, quint64& sinceTime,
+    static quint64 getAdjustedConsiderSince(quint64 sinceTime);
-                                                         bool& hasMore);
+
+    QMultiMap<quint64, QUuid> getRecentlyDeletedEntityIDs() const { 
+        QReadLocker locker(&_recentlyDeletedEntitiesLock);
+        return _recentlyDeletedEntityItemIDs;
+    }
+
     void forgetEntitiesDeletedBefore(quint64 sinceTime);
 
     int processEraseMessage(NLPacket& packet, const SharedNodePointer& sourceNode);
@@ -243,7 +252,7 @@ private:
     QReadWriteLock _newlyCreatedHooksLock;
     QVector<NewlyCreatedEntityHook*> _newlyCreatedHooks;
 
-    QReadWriteLock _recentlyDeletedEntitiesLock;
+    mutable QReadWriteLock _recentlyDeletedEntitiesLock;
     QMultiMap<quint64, QUuid> _recentlyDeletedEntityItemIDs;
     EntityItemFBXService* _fbxService;