From c210b0db1c289560c64228d45a12657cab484877 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 14 Jul 2015 20:18:28 -0700 Subject: [PATCH 1/7] don't double seek in Packet::writeData --- libraries/networking/src/udt/Packet.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libraries/networking/src/udt/Packet.cpp b/libraries/networking/src/udt/Packet.cpp index 9645c5afcc..1b53756f5e 100644 --- a/libraries/networking/src/udt/Packet.cpp +++ b/libraries/networking/src/udt/Packet.cpp @@ -196,15 +196,14 @@ qint64 Packet::writeData(const char* data, qint64 maxSize) { // make sure we have the space required to write this block if (maxSize <= bytesAvailableForWrite()) { qint64 currentPos = pos(); + + Q_ASSERT(currentPos < _capacity); // good to go - write the data memcpy(_payloadStart + currentPos, data, maxSize); - // seek to the new position based on where our write just finished - seek(currentPos + maxSize); - // keep track of _sizeUsed so we can just write the actual data when packet is about to be sent - _sizeUsed = std::max(pos(), _sizeUsed); + _sizeUsed = std::max(currentPos + maxSize, _sizeUsed); // return the number of bytes written return maxSize; From 0327a8d4777e381f64565cfd7ec25c302bfb4804 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 14 Jul 2015 20:47:16 -0700 Subject: [PATCH 2/7] fix bytesAvailable, don't double seek in Packet --- assignment-client/src/octree/OctreeQueryNode.cpp | 2 +- assignment-client/src/octree/OctreeQueryNode.h | 2 +- assignment-client/src/octree/OctreeSendThread.cpp | 2 +- libraries/entities/src/EntityTree.cpp | 2 +- libraries/networking/src/NodeList.cpp | 4 ++-- libraries/networking/src/udt/Packet.cpp | 5 +---- libraries/networking/src/udt/PacketList.cpp | 6 +++--- libraries/octree/src/OctreeEditPacketSender.cpp | 2 +- 8 files changed, 11 insertions(+), 14 deletions(-) diff --git a/assignment-client/src/octree/OctreeQueryNode.cpp b/assignment-client/src/octree/OctreeQueryNode.cpp index 2f1a554b8c..65ee0e26f8 100644 --- a/assignment-client/src/octree/OctreeQueryNode.cpp +++ b/assignment-client/src/octree/OctreeQueryNode.cpp @@ -218,7 +218,7 @@ void OctreeQueryNode::writeToPacket(const unsigned char* buffer, unsigned int by OCTREE_PACKET_INTERNAL_SECTION_SIZE sectionSize = bytes; _octreePacket->writePrimitive(sectionSize); } - if (bytes <= _octreePacket->bytesAvailable()) { + if (bytes <= _octreePacket->bytesAvailableForWrite()) { _octreePacket->write(reinterpret_cast(buffer), bytes); _octreePacketWaiting = true; } diff --git a/assignment-client/src/octree/OctreeQueryNode.h b/assignment-client/src/octree/OctreeQueryNode.h index 6316cc5e86..d752b8d0e8 100644 --- a/assignment-client/src/octree/OctreeQueryNode.h +++ b/assignment-client/src/octree/OctreeQueryNode.h @@ -46,7 +46,7 @@ public: bool packetIsDuplicate() const; bool shouldSuppressDuplicatePacket(); - unsigned int getAvailable() const { return _octreePacket->bytesAvailable(); } + unsigned int getAvailable() const { return _octreePacket->bytesAvailableForWrite(); } int getMaxSearchLevel() const { return _maxSearchLevel; } void resetMaxSearchLevel() { _maxSearchLevel = 1; } void incrementMaxSearchLevel() { _maxSearchLevel++; } diff --git a/assignment-client/src/octree/OctreeSendThread.cpp b/assignment-client/src/octree/OctreeSendThread.cpp index 6a5d76d4b9..7937604825 100644 --- a/assignment-client/src/octree/OctreeSendThread.cpp +++ b/assignment-client/src/octree/OctreeSendThread.cpp @@ -147,7 +147,7 @@ int OctreeSendThread::handlePacketSend(OctreeQueryNode* nodeData, int& trueBytes NLPacket& statsPacket = nodeData->stats.getStatsMessage(); // If the size of the stats message and the octree message will fit in a packet, then piggyback them - if (nodeData->getPacket().getSizeWithHeader() <= statsPacket.bytesAvailable()) { + if (nodeData->getPacket().getSizeWithHeader() <= statsPacket.bytesAvailableForWrite()) { // copy octree message to back of stats message statsPacket.write(nodeData->getPacket().getData(), nodeData->getPacket().getSizeWithHeader()); diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index a5c5d58936..cb4011c799 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -796,7 +796,7 @@ std::unique_ptr EntityTree::encodeEntitiesDeletedSince(OCTREE_PACKET_S ++numberOfIDs; // check to make sure we have room for one more ID - if (NUM_BYTES_RFC4122_UUID > deletesPacket->bytesAvailable()) { + if (NUM_BYTES_RFC4122_UUID > deletesPacket->bytesAvailableForWrite()) { hasFilledPacket = true; break; } diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 89013ece60..58720afe2c 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -284,7 +284,7 @@ void NodeList::sendDomainServerCheckIn() { flagTimeForConnectionStep(LimitedNodeList::ConnectionStep::SendDSCheckIn); - if (!isUsingDTLS) { + if (!isUsingDTLS) { sendPacket(std::move(domainPacket), _domainHandler.getSockAddr()); } @@ -336,7 +336,7 @@ void NodeList::sendDSPathQuery(const QString& newPath) { // get the size of the UTF8 representation of the desired path qint64 numPathBytes = pathQueryUTF8.size(); - if (numPathBytes + ((qint64) sizeof(numPathBytes)) < pathQueryPacket->bytesAvailable()) { + if (numPathBytes + ((qint64) sizeof(numPathBytes)) < pathQueryPacket->bytesAvailableForWrite()) { // append the size of the path to the query packet pathQueryPacket->writePrimitive(numPathBytes); diff --git a/libraries/networking/src/udt/Packet.cpp b/libraries/networking/src/udt/Packet.cpp index 1b53756f5e..975c137f27 100644 --- a/libraries/networking/src/udt/Packet.cpp +++ b/libraries/networking/src/udt/Packet.cpp @@ -70,7 +70,7 @@ Packet::Packet(PacketType::Value type, qint64 size) : } _packetSize = localHeaderSize(type) + size; - _packet.reset(new char(_packetSize)); + _packet.reset(new char[_packetSize]); _capacity = size; _payloadStart = _packet.get() + (_packetSize - _capacity); @@ -222,9 +222,6 @@ qint64 Packet::readData(char* dest, qint64 maxSize) { // read out the data memcpy(dest, _payloadStart + currentPosition, numBytesToRead); - - // seek to the end of the read - seek(currentPosition + numBytesToRead); } return numBytesToRead; diff --git a/libraries/networking/src/udt/PacketList.cpp b/libraries/networking/src/udt/PacketList.cpp index e48ffa0f69..2c41f61d0f 100644 --- a/libraries/networking/src/udt/PacketList.cpp +++ b/libraries/networking/src/udt/PacketList.cpp @@ -56,7 +56,7 @@ qint64 PacketList::writeData(const char* data, qint64 maxSize) { } // check if this block of data can fit into the currentPacket - if (maxSize <= _currentPacket->bytesAvailable()) { + if (maxSize <= _currentPacket->bytesAvailableForWrite()) { // it fits, just write it to the current packet _currentPacket->write(data, maxSize); @@ -73,7 +73,7 @@ qint64 PacketList::writeData(const char* data, qint64 maxSize) { // We need to try and pull the first part of the segment out to our new packet // check now to see if this is an unsupported write - int numBytesToEnd = _currentPacket->bytesAvailable(); + int numBytesToEnd = _currentPacket->bytesAvailableForWrite(); if ((newPacket->size() - numBytesToEnd) < maxSize) { // this is an unsupported case - the segment is bigger than the size of an individual packet @@ -108,7 +108,7 @@ qint64 PacketList::writeData(const char* data, qint64 maxSize) { // we're an ordered PacketList - let's fit what we can into the current packet and then put the leftover // into a new packet - int numBytesToEnd = _currentPacket->bytesAvailable(); + int numBytesToEnd = _currentPacket->bytesAvailableForWrite(); _currentPacket->write(data, numBytesToEnd); // move the current packet to our list of packets diff --git a/libraries/octree/src/OctreeEditPacketSender.cpp b/libraries/octree/src/OctreeEditPacketSender.cpp index 6bbfdc5024..6f2780d871 100644 --- a/libraries/octree/src/OctreeEditPacketSender.cpp +++ b/libraries/octree/src/OctreeEditPacketSender.cpp @@ -255,7 +255,7 @@ void OctreeEditPacketSender::queueOctreeEditMessage(PacketType::Value type, QByt } else { // If we're switching type, then we send the last one and start over if ((type != bufferedPacket->getType() && bufferedPacket->getSizeUsed() > 0) || - (editMessage.size() >= bufferedPacket->bytesAvailable())) { + (editMessage.size() >= bufferedPacket->bytesAvailableForWrite())) { // create the new packet and swap it with the packet in _pendingEditPackets auto packetToRelease = initializePacket(type, node->getClockSkewUsec()); From 89c44ded38a2bce88bdf9dcc127ee09f0d713f04 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 14 Jul 2015 21:01:26 -0700 Subject: [PATCH 3/7] repairs for domain-server PacketList sending --- domain-server/src/DomainServer.cpp | 3 +++ libraries/networking/src/LimitedNodeList.cpp | 11 ++++++++--- libraries/networking/src/udt/PacketList.cpp | 12 +++++++++--- libraries/networking/src/udt/PacketList.h | 2 +- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index b493684f45..4023fbcd1f 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -1007,6 +1007,9 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif }); } } + + // send an empty list to the node, in case there were no other nodes + domainListPackets.closeCurrentPacket(true); // write the PacketList to this node limitedNodeList->sendPacketList(domainListPackets, *node); diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index fe4efb3de1..33139e66e8 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -263,15 +263,20 @@ qint64 LimitedNodeList::sendPacket(std::unique_ptr packet, const HifiS } qint64 LimitedNodeList::sendPacketList(NLPacketList& packetList, const Node& destinationNode) { - if (!destinationNode.getActiveSocket()) { + const HifiSockAddr* activeSocket = destinationNode.getActiveSocket(); + if (!activeSocket) { // we don't have a socket to send to, return 0 return 0; } - return sendPacketList(packetList, *destinationNode.getActiveSocket()); + return sendPacketList(packetList, *activeSocket); } qint64 LimitedNodeList::sendPacketList(NLPacketList& packetList, const HifiSockAddr& sockAddr) { - qint64 bytesSent{ 0 }; + qint64 bytesSent { 0 }; + + // close the last packet in the list + packetList.closeCurrentPacket(); + while (!packetList._packets.empty()) { bytesSent += sendPacket(std::move(packetList.takeFront()), sockAddr); } diff --git a/libraries/networking/src/udt/PacketList.cpp b/libraries/networking/src/udt/PacketList.cpp index 2c41f61d0f..675645abc2 100644 --- a/libraries/networking/src/udt/PacketList.cpp +++ b/libraries/networking/src/udt/PacketList.cpp @@ -120,7 +120,13 @@ qint64 PacketList::writeData(const char* data, qint64 maxSize) { } } -void PacketList::closeCurrentPacket() { - // move the current packet to our list of packets - _packets.push_back(std::move(_currentPacket)); +void PacketList::closeCurrentPacket(bool shouldSendEmpty) { + if (shouldSendEmpty && !_currentPacket) { + _currentPacket = createPacketWithExtendedHeader(); + } + + if (_currentPacket) { + // move the current packet to our list of packets + _packets.push_back(std::move(_currentPacket)); + } } diff --git a/libraries/networking/src/udt/PacketList.h b/libraries/networking/src/udt/PacketList.h index 173e8c75fb..f4b542155a 100644 --- a/libraries/networking/src/udt/PacketList.h +++ b/libraries/networking/src/udt/PacketList.h @@ -31,7 +31,7 @@ public: PacketType::Value getType() const { return _packetType; } int getNumPackets() const { return _packets.size() + (_currentPacket ? 1 : 0); } - void closeCurrentPacket(); + void closeCurrentPacket(bool shouldSendEmpty = false); void setExtendedHeader(const QByteArray& extendedHeader) { _extendedHeader = extendedHeader; } From ccc2649d264950a003d7f131530dff32c57e5bcb Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 14 Jul 2015 21:14:15 -0700 Subject: [PATCH 4/7] fix packing of domain list header --- domain-server/src/DomainServer.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 4023fbcd1f..1824446fca 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -971,10 +971,10 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif // setup the extended header for the domain list packets // this data is at the beginning of each of the domain list packets QByteArray extendedHeader(NUM_DOMAIN_LIST_EXTENDED_HEADER_BYTES, 0); - QDataStream extendedHeaderStream(&extendedHeader, QIODevice::Append); + QDataStream extendedHeaderStream(&extendedHeader, QIODevice::WriteOnly); - extendedHeaderStream << limitedNodeList->getSessionUUID().toRfc4122(); - extendedHeaderStream << node->getUUID().toRfc4122(); + extendedHeaderStream << limitedNodeList->getSessionUUID(); + extendedHeaderStream << node->getUUID(); extendedHeaderStream << (quint8) node->getCanAdjustLocks(); extendedHeaderStream << (quint8) node->getCanRez(); From e00dc8bfaac6f7dd29bca66adef99dab007f1124 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 14 Jul 2015 21:15:22 -0700 Subject: [PATCH 5/7] remove an Xcode vim accidentalism --- libraries/networking/src/NodeList.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 58720afe2c..c6d46e88a7 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -473,6 +473,7 @@ void NodeList::processDomainServerList(QSharedPointer packet) { quint8 thisNodeCanRez; packetStream >> thisNodeCanRez; setThisNodeCanRez((bool) thisNodeCanRez); + // pull each node in the packet while (packetStream.device()->pos() < packet->getSizeUsed()) { From 9204f004c0f7e057ba19246eb86dea4b826a57d4 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 14 Jul 2015 21:15:39 -0700 Subject: [PATCH 6/7] remove an extra line --- libraries/networking/src/NodeList.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index c6d46e88a7..dbcc4c3387 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -474,7 +474,6 @@ void NodeList::processDomainServerList(QSharedPointer packet) { packetStream >> thisNodeCanRez; setThisNodeCanRez((bool) thisNodeCanRez); - // pull each node in the packet while (packetStream.device()->pos() < packet->getSizeUsed()) { parseNodeFromPacketStream(packetStream); From 7d313c4a2b696c28ec5ea88467e84b0aa484f3fc Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 15 Jul 2015 10:13:57 -0700 Subject: [PATCH 7/7] remove extra debugs in PacketReceiver --- libraries/networking/src/PacketReceiver.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/libraries/networking/src/PacketReceiver.cpp b/libraries/networking/src/PacketReceiver.cpp index 7af4905834..d7d2fdcd6b 100644 --- a/libraries/networking/src/PacketReceiver.cpp +++ b/libraries/networking/src/PacketReceiver.cpp @@ -243,13 +243,11 @@ void PacketReceiver::processDatagrams() { static const QByteArray SHARED_NODE_NORMALIZED = QMetaObject::normalizedType("SharedNodePointer"); if (metaMethod.parameterTypes().contains(SHARED_NODE_NORMALIZED)) { - qDebug() << "invoking with matchingNode" << matchingNode; success = metaMethod.invoke(listener.first, Q_ARG(QSharedPointer, QSharedPointer(packet.release())), Q_ARG(SharedNodePointer, matchingNode)); } else if (metaMethod.parameterTypes().contains(QSHAREDPOINTER_NODE_NORMALIZED)) { - qDebug() << "invoking with matchingNode" << matchingNode; success = metaMethod.invoke(listener.first, Q_ARG(QSharedPointer, QSharedPointer(packet.release())), Q_ARG(QSharedPointer, matchingNode));