From 929e829801c4e644af2114867f82496aa6b4a9d4 Mon Sep 17 00:00:00 2001 From: Anthony Thibault Date: Wed, 13 Nov 2019 17:17:37 -0800 Subject: [PATCH] Revert "Speculative fix for infinite loop in SafeLanding" This reverts commit 52acfc9333cb527dc97d940d65da976d1af1ab76. --- interface/src/octree/SafeLanding.cpp | 75 ++++++++++++---------------- interface/src/octree/SafeLanding.h | 4 +- 2 files changed, 34 insertions(+), 45 deletions(-) diff --git a/interface/src/octree/SafeLanding.cpp b/interface/src/octree/SafeLanding.cpp index bcf345bae8..fbc35f2732 100644 --- a/interface/src/octree/SafeLanding.cpp +++ b/interface/src/octree/SafeLanding.cpp @@ -39,7 +39,6 @@ bool SafeLanding::SequenceLessThan::operator()(const OCTREE_PACKET_SEQUENCE& a, void SafeLanding::startTracking(QSharedPointer entityTreeRenderer) { if (!entityTreeRenderer.isNull()) { - qCDebug(interfaceapp) << "SafeLanding has started tracking"; auto entityTree = entityTreeRenderer->getTree(); if (entityTree && !_trackingEntities) { Locker lock(_lock); @@ -105,55 +104,47 @@ void SafeLanding::updateTracking() { return; } - Locker lock(_lock); - - bool enableInterstitial = DependencyManager::get()->getDomainHandler().getInterstitialModeEnabled(); - auto entityMapIter = _trackedEntities.begin(); - while (entityMapIter != _trackedEntities.end()) { - auto entity = entityMapIter->second; - bool isVisuallyReady = true; - if (enableInterstitial) { - auto entityRenderable = _entityTreeRenderer->renderableForEntityId(entityMapIter->first); - if (!entityRenderable) { - _entityTreeRenderer->addingEntity(entityMapIter->first); + { + Locker lock(_lock); + bool enableInterstitial = DependencyManager::get()->getDomainHandler().getInterstitialModeEnabled(); + auto entityMapIter = _trackedEntities.begin(); + while (entityMapIter != _trackedEntities.end()) { + auto entity = entityMapIter->second; + bool isVisuallyReady = true; + if (enableInterstitial) { + auto entityRenderable = _entityTreeRenderer->renderableForEntityId(entityMapIter->first); + if (!entityRenderable) { + _entityTreeRenderer->addingEntity(entityMapIter->first); + } + isVisuallyReady = entity->isVisuallyReady() || (!entityRenderable && !entity->isParentPathComplete()); + } + if (isEntityPhysicsReady(entity) && isVisuallyReady) { + entityMapIter = _trackedEntities.erase(entityMapIter); + } else { + entityMapIter++; } - isVisuallyReady = entity->isVisuallyReady() || (!entityRenderable && !entity->isParentPathComplete()); } - if (isEntityPhysicsReady(entity) && isVisuallyReady) { - entityMapIter = _trackedEntities.erase(entityMapIter); - } else { - entityMapIter++; + if (enableInterstitial) { + _trackedEntityStabilityCount++; } } - if (enableInterstitial) { - _trackedEntityStabilityCount++; - } if (_trackedEntities.empty()) { // no more tracked entities --> check sequenceNumbers if (_sequenceStart != SafeLanding::INVALID_SEQUENCE) { bool shouldStop = false; - auto sequenceSize = _sequenceEnd - _sequenceStart; // this works even in rollover case - auto startIter = _sequenceNumbers.find(_sequenceStart); - auto endIter = _sequenceNumbers.find(_sequenceEnd - 1); + { + Locker lock(_lock); + auto sequenceSize = _sequenceEnd - _sequenceStart; // this works even in rollover case + auto startIter = _sequenceNumbers.find(_sequenceStart); + auto endIter = _sequenceNumbers.find(_sequenceEnd - 1); - bool missingSequenceNumbers = qApp->isMissingSequenceNumbers(); - - // If the EntityQueryInitialResultsComplete packet is really late due to packet loss, the - // _sequenceNumbers map will be filled with unnecessary sequence numbers. This can cause - // the main thread to enter an infinite loop in the std::distance() calculation. - // Try to guard against this. This might cause physics to be enabled too soon, but - // that is preferable to locking up. - bool tooManySequenceNumbers = _sequenceNumbers.size() >= (std::numeric_limits::max() / 2); - - qCDebug(interfaceapp) << "SafeLanding has no more tracked entities and" << _sequenceNumbers.size() << "sequence numbers"; - - shouldStop = (sequenceSize == 0 || + bool missingSequenceNumbers = qApp->isMissingSequenceNumbers(); + shouldStop = (sequenceSize == 0 || (startIter != _sequenceNumbers.end() && endIter != _sequenceNumbers.end() && - !tooManySequenceNumbers && ((distance(startIter, endIter) == sequenceSize - 1) || !missingSequenceNumbers))); - + } if (shouldStop) { stopTracking(); } @@ -162,8 +153,6 @@ void SafeLanding::updateTracking() { } void SafeLanding::stopTracking() { - qCDebug(interfaceapp) << "SafeLanding has stopped tracking"; - Locker lock(_lock); if (_trackingEntities) { _trackingEntities = false; @@ -180,7 +169,6 @@ void SafeLanding::stopTracking() { } void SafeLanding::reset() { - Locker lock(_lock); _trackingEntities = false; _trackedEntities.clear(); _maxTrackedEntityCount = 0; @@ -189,7 +177,6 @@ void SafeLanding::reset() { } bool SafeLanding::trackingIsComplete() const { - Locker lock(_lock); return !_trackingEntities && (_sequenceStart != SafeLanding::INVALID_SEQUENCE); } @@ -254,8 +241,10 @@ void SafeLanding::debugDumpSequenceIDs() const { ++itr; while (itr != _sequenceNumbers.end()) { OCTREE_PACKET_SEQUENCE s = *itr; - qCDebug(interfaceapp) << " " << (int32_t)s; - p = s; + if (s != p + 1) { + qCDebug(interfaceapp) << "Gap from" << (int32_t)p << "to" << (int32_t)s << "(exclusive)"; + p = s; + } ++itr; } if (p != SafeLanding::INVALID_SEQUENCE) { diff --git a/interface/src/octree/SafeLanding.h b/interface/src/octree/SafeLanding.h index d3131752bf..00473f6fc8 100644 --- a/interface/src/octree/SafeLanding.h +++ b/interface/src/octree/SafeLanding.h @@ -51,8 +51,8 @@ private: bool isEntityPhysicsReady(const EntityItemPointer& entity); void debugDumpSequenceIDs() const; - mutable std::recursive_mutex _lock; - using Locker = std::lock_guard; + std::mutex _lock; + using Locker = std::lock_guard; bool _trackingEntities { false }; QSharedPointer _entityTreeRenderer; using EntityMap = std::map;