From cc6b0adb7d1263fe5eb84d3b16919655ea6f9288 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 30 Sep 2015 14:13:36 -0400 Subject: [PATCH 1/5] guard insert/get in SentPacketHistory --- libraries/networking/src/SentPacketHistory.cpp | 6 ++++-- libraries/networking/src/SentPacketHistory.h | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/libraries/networking/src/SentPacketHistory.cpp b/libraries/networking/src/SentPacketHistory.cpp index c6eec8eb63..d0fcecd171 100644 --- a/libraries/networking/src/SentPacketHistory.cpp +++ b/libraries/networking/src/SentPacketHistory.cpp @@ -35,10 +35,11 @@ void SentPacketHistory::packetSent(uint16_t sequenceNumber, const NLPacket& pack } _newestSequenceNumber = sequenceNumber; + QWriteLocker locker(&_packetsLock); _sentPackets.insert(NLPacket::createCopy(packet)); } -const NLPacket* SentPacketHistory::getPacket(uint16_t sequenceNumber) const { +const NLPacket* SentPacketHistory::getPacket(uint16_t sequenceNumber) { const int UINT16_RANGE = std::numeric_limits::max() + 1; @@ -48,6 +49,7 @@ const NLPacket* SentPacketHistory::getPacket(uint16_t sequenceNumber) const { if (seqDiff < 0) { seqDiff += UINT16_RANGE; } - + + QReadLocker locker(&_packetsLock); return _sentPackets.get(seqDiff)->get(); } diff --git a/libraries/networking/src/SentPacketHistory.h b/libraries/networking/src/SentPacketHistory.h index 1808e0020b..78b6914c2d 100644 --- a/libraries/networking/src/SentPacketHistory.h +++ b/libraries/networking/src/SentPacketHistory.h @@ -12,7 +12,9 @@ #define hifi_SentPacketHistory_h #include -#include + +#include +#include #include "NLPacket.h" #include "RingBufferHistory.h" @@ -26,9 +28,10 @@ public: SentPacketHistory(int size = MAX_REASONABLE_SEQUENCE_GAP); void packetSent(uint16_t sequenceNumber, const NLPacket& packet); - const NLPacket* getPacket(uint16_t sequenceNumber) const; + const NLPacket* getPacket(uint16_t sequenceNumber); private: + QReadWriteLock _packetsLock { QReadWriteLock::Recursive }; RingBufferHistory> _sentPackets; // circular buffer uint16_t _newestSequenceNumber; From f7e7b07441e014f3875cb1db0440252cee354342 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 30 Sep 2015 14:15:22 -0400 Subject: [PATCH 2/5] fix constness of SentPacketHistory in OEPS --- libraries/networking/src/NLPacketList.cpp | 3 ++- libraries/octree/src/OctreeEditPacketSender.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/networking/src/NLPacketList.cpp b/libraries/networking/src/NLPacketList.cpp index 3b115c558b..318fb037a1 100644 --- a/libraries/networking/src/NLPacketList.cpp +++ b/libraries/networking/src/NLPacketList.cpp @@ -23,7 +23,8 @@ std::unique_ptr NLPacketList::create(PacketType packetType, QByteA } std::unique_ptr NLPacketList::fromPacketList(std::unique_ptr packetList) { - auto nlPacketList = std::unique_ptr(new NLPacketList(std::move(*packetList.release()))); nlPacketList->open(ReadOnly); + auto nlPacketList = std::unique_ptr(new NLPacketList(std::move(*packetList.release()))); + nlPacketList->open(ReadOnly); return nlPacketList; } diff --git a/libraries/octree/src/OctreeEditPacketSender.cpp b/libraries/octree/src/OctreeEditPacketSender.cpp index 1be271cbdd..d4bd939196 100644 --- a/libraries/octree/src/OctreeEditPacketSender.cpp +++ b/libraries/octree/src/OctreeEditPacketSender.cpp @@ -346,7 +346,7 @@ void OctreeEditPacketSender::processNackPacket(NLPacket& packet, SharedNodePoint if (_sentPacketHistories.count(sendingNode->getUUID()) == 0) { return; } - const SentPacketHistory& sentPacketHistory = _sentPacketHistories[sendingNode->getUUID()]; + SentPacketHistory& sentPacketHistory = _sentPacketHistories[sendingNode->getUUID()]; // read sequence numbers and queue packets for resend while (packet.bytesLeftToRead() > 0) { From 82ac0b1a27da3600dfca483c39ef495e877ce384 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 30 Sep 2015 14:23:18 -0400 Subject: [PATCH 3/5] use a mutable mutex to keep const-ness --- libraries/networking/src/SentPacketHistory.cpp | 2 +- libraries/networking/src/SentPacketHistory.h | 4 ++-- libraries/octree/src/OctreeEditPacketSender.cpp | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/libraries/networking/src/SentPacketHistory.cpp b/libraries/networking/src/SentPacketHistory.cpp index d0fcecd171..14b4677fc3 100644 --- a/libraries/networking/src/SentPacketHistory.cpp +++ b/libraries/networking/src/SentPacketHistory.cpp @@ -39,7 +39,7 @@ void SentPacketHistory::packetSent(uint16_t sequenceNumber, const NLPacket& pack _sentPackets.insert(NLPacket::createCopy(packet)); } -const NLPacket* SentPacketHistory::getPacket(uint16_t sequenceNumber) { +const NLPacket* SentPacketHistory::getPacket(uint16_t sequenceNumber) const { const int UINT16_RANGE = std::numeric_limits::max() + 1; diff --git a/libraries/networking/src/SentPacketHistory.h b/libraries/networking/src/SentPacketHistory.h index 78b6914c2d..72150658e3 100644 --- a/libraries/networking/src/SentPacketHistory.h +++ b/libraries/networking/src/SentPacketHistory.h @@ -28,10 +28,10 @@ public: SentPacketHistory(int size = MAX_REASONABLE_SEQUENCE_GAP); void packetSent(uint16_t sequenceNumber, const NLPacket& packet); - const NLPacket* getPacket(uint16_t sequenceNumber); + const NLPacket* getPacket(uint16_t sequenceNumber) const; private: - QReadWriteLock _packetsLock { QReadWriteLock::Recursive }; + mutable QReadWriteLock _packetsLock { QReadWriteLock::Recursive }; RingBufferHistory> _sentPackets; // circular buffer uint16_t _newestSequenceNumber; diff --git a/libraries/octree/src/OctreeEditPacketSender.cpp b/libraries/octree/src/OctreeEditPacketSender.cpp index d4bd939196..495effc825 100644 --- a/libraries/octree/src/OctreeEditPacketSender.cpp +++ b/libraries/octree/src/OctreeEditPacketSender.cpp @@ -346,7 +346,8 @@ void OctreeEditPacketSender::processNackPacket(NLPacket& packet, SharedNodePoint if (_sentPacketHistories.count(sendingNode->getUUID()) == 0) { return; } - SentPacketHistory& sentPacketHistory = _sentPacketHistories[sendingNode->getUUID()]; + + const SentPacketHistory& sentPacketHistory = _sentPacketHistories[sendingNode->getUUID()]; // read sequence numbers and queue packets for resend while (packet.bytesLeftToRead() > 0) { From 1e9593029c260c5c346479972e13be7417c3a038 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 30 Sep 2015 11:32:41 -0700 Subject: [PATCH 4/5] Fix infinite recursion error with CC rng On some std::random implementations tryin get a range [low, high) where high < low will cause infinite recursion. --- .../networking/src/udt/CongestionControl.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/libraries/networking/src/udt/CongestionControl.cpp b/libraries/networking/src/udt/CongestionControl.cpp index ea46d60acb..c1feae3911 100644 --- a/libraries/networking/src/udt/CongestionControl.cpp +++ b/libraries/networking/src/udt/CongestionControl.cpp @@ -161,13 +161,16 @@ void DefaultCC::onLoss(SequenceNumber rangeStart, SequenceNumber rangeEnd) { _lastDecreaseMaxSeq = _sendCurrSeqNum; - // avoid synchronous rate decrease across connections using randomization - std::random_device rd; - std::mt19937 generator(rd()); - std::uniform_int_distribution<> distribution(1, _avgNAKNum); - - _randomDecreaseThreshold = distribution(generator); - + if (_avgNAKNum < 1) { + _randomDecreaseThreshold = 1; + } else { + // avoid synchronous rate decrease across connections using randomization + std::random_device rd; + std::mt19937 generator(rd()); + std::uniform_int_distribution<> distribution(1, std::max(1, _avgNAKNum)); + + _randomDecreaseThreshold = distribution(generator); + } } else if ((_decreaseCount++ < MAX_DECREASES_PER_CONGESTION_EPOCH) && ((++_nakCount % _randomDecreaseThreshold) == 0)) { // there have been fewer than MAX_DECREASES_PER_CONGESTION_EPOCH AND this NAK matches the random count at which we // decided we would decrease the packet send period From 83c3f65b1f6c8f92c4cf2c9681ab1d307d84f4a4 Mon Sep 17 00:00:00 2001 From: "Anthony J. Thibault" Date: Wed, 30 Sep 2015 11:34:38 -0700 Subject: [PATCH 5/5] Prevent taking a reference to a QUrl rvalue. --- libraries/avatars/src/AvatarData.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 3bd147d398..f64504d662 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -966,7 +966,8 @@ bool AvatarData::hasIdentityChangedAfterParsing(NLPacket& packet) { QByteArray AvatarData::identityByteArray() { QByteArray identityData; QDataStream identityStream(&identityData, QIODevice::Append); - const QUrl& urlToSend = (_skeletonModelURL == AvatarData::defaultFullAvatarModelUrl()) ? QUrl("") : _skeletonModelURL; + QUrl emptyURL(""); + const QUrl& urlToSend = (_skeletonModelURL == AvatarData::defaultFullAvatarModelUrl()) ? emptyURL : _skeletonModelURL; identityStream << QUuid() << _faceModelURL << urlToSend << _attachmentData << _displayName;