From ecaa50c0ff82de10c2714b26cf0fac8c1a51cb9b Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Thu, 12 Nov 2015 18:25:18 -0800 Subject: [PATCH] fix for infinite loop in erase entities special packet --- .../src/entities/EntityServer.cpp | 114 ++++++++++++++++-- libraries/entities/src/EntityTree.cpp | 89 +------------- libraries/entities/src/EntityTree.h | 17 ++- 3 files changed, 125 insertions(+), 95 deletions(-) diff --git a/assignment-client/src/entities/EntityServer.cpp b/assignment-client/src/entities/EntityServer.cpp index 493a16fea4..4448a3c5c3 100644 --- a/assignment-client/src/entities/EntityServer.cpp +++ b/assignment-client/src/entities/EntityServer.cpp @@ -101,24 +101,121 @@ int EntityServer::sendSpecialPackets(const SharedNodePointer& node, OctreeQueryN EntityNodeData* nodeData = static_cast(node->getLinkedData()); if (nodeData) { + quint64 deletedEntitiesSentAt = nodeData->getLastDeletedEntitiesSentAt(); + quint64 considerEntitiesSince = EntityTree::getAdjustedConsiderSince(deletedEntitiesSentAt); + quint64 deletePacketSentAt = usecTimestampNow(); EntityTreePointer tree = std::static_pointer_cast(_tree); + auto recentlyDeleted = tree->getRecentlyDeletedEntityIDs(); bool hasMoreToSend = true; packetsSent = 0; - while (hasMoreToSend) { - auto specialPacket = tree->encodeEntitiesDeletedSince(queryNode->getSequenceNumber(), deletedEntitiesSentAt, - hasMoreToSend); + // create a new special packet + std::unique_ptr deletesPacket = NLPacket::create(PacketType::EntityErase); - queryNode->packetSent(*specialPacket); + // pack in flags + OCTREE_PACKET_FLAGS flags = 0; + deletesPacket->writePrimitive(flags); - totalBytes += specialPacket->getDataSize(); - packetsSent++; + // pack in sequence number + auto sequenceNumber = queryNode->getSequenceNumber(); + deletesPacket->writePrimitive(sequenceNumber); - DependencyManager::get()->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()->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()->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(_tree); if (tree->hasAnyDeletedEntities()) { diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 0174dbe39b..c2f635c95f 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/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 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 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; diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index c177840199..645e3f4f76 100644 --- a/libraries/entities/src/EntityTree.h +++ b/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 encodeEntitiesDeletedSince(OCTREE_PACKET_SEQUENCE sequenceNumber, quint64& sinceTime, - bool& hasMore); + static quint64 getAdjustedConsiderSince(quint64 sinceTime); + + QMultiMap 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 _newlyCreatedHooks; - QReadWriteLock _recentlyDeletedEntitiesLock; + mutable QReadWriteLock _recentlyDeletedEntitiesLock; QMultiMap _recentlyDeletedEntityItemIDs; EntityItemFBXService* _fbxService;