From f8a24a2e995c5b6f51c72f7d888cb5eaab96fd8d Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Mon, 10 Feb 2014 12:19:46 -0800 Subject: [PATCH 1/2] fix crash in multiple voxel edit scripts running simultaneously --- examples/gameoflife.js | 1 - libraries/shared/src/NodeList.cpp | 2 +- .../voxels/src/VoxelEditPacketSender.cpp | 23 +++++-------------- 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/examples/gameoflife.js b/examples/gameoflife.js index 6779941dc7..d72a72c2de 100644 --- a/examples/gameoflife.js +++ b/examples/gameoflife.js @@ -115,7 +115,6 @@ function sendNextCells() { var sentFirstBoard = false; function step() { -print("step()..."); if (sentFirstBoard) { // we've already sent the first full board, perform a step in time updateCells(); diff --git a/libraries/shared/src/NodeList.cpp b/libraries/shared/src/NodeList.cpp index 2672f3b27b..18b6e9883e 100644 --- a/libraries/shared/src/NodeList.cpp +++ b/libraries/shared/src/NodeList.cpp @@ -87,7 +87,7 @@ bool NodeList::packetVersionAndHashMatch(const QByteArray& packet) { if (packet[1] != versionForPacketType(packetTypeForPacket(packet)) && packetTypeForPacket(packet) != PacketTypeStunResponse) { PacketType mismatchType = packetTypeForPacket(packet); - int numPacketTypeBytes = arithmeticCodingValueFromBuffer(packet.data()); + int numPacketTypeBytes = numBytesArithmeticCodingFromBuffer(packet.data()); qDebug() << "Packet version mismatch on" << packetTypeForPacket(packet) << "- Sender" << uuidFromPacketHeader(packet) << "sent" << qPrintable(QString::number(packet[numPacketTypeBytes])) << "but" diff --git a/libraries/voxels/src/VoxelEditPacketSender.cpp b/libraries/voxels/src/VoxelEditPacketSender.cpp index fea75abf23..a6d3668207 100644 --- a/libraries/voxels/src/VoxelEditPacketSender.cpp +++ b/libraries/voxels/src/VoxelEditPacketSender.cpp @@ -14,24 +14,13 @@ #include #include "VoxelEditPacketSender.h" -////////////////////////////////////////////////////////////////////////////////////////// -// Function: createVoxelEditMessage() -// Description: creates an "insert" or "remove" voxel message for a voxel code -// corresponding to the closest voxel which encloses a cube with -// lower corners at x,y,z, having side of length S. -// The input values x,y,z range 0.0 <= v < 1.0 -// message should be either 'S' for SET or 'E' for ERASE -// -// IMPORTANT: The buffer is returned to you a buffer which you MUST delete when you are -// done with it. -// -// HACK ATTACK: Well, what if this is larger than the MTU? That's the caller's problem, we -// just truncate the message -// -// Complaints: Brad :) #define GUESS_OF_VOXELCODE_SIZE 10 #define MAXIMUM_EDIT_VOXEL_MESSAGE_SIZE 1500 #define SIZE_OF_COLOR_DATA sizeof(rgbColor) +/// creates an "insert" or "remove" voxel message for a voxel code corresponding to the closest voxel which encloses a cube +/// with lower corners at x,y,z, having side of length S. The input values x,y,z range 0.0 <= v < 1.0 message should be either +/// PacketTypeVoxelSet, PacketTypeVoxelSetDestructive, or PacketTypeVoxelErase. The buffer is returned to caller becomes +/// responsibility of caller and MUST be deleted by caller. bool createVoxelEditMessage(PacketType command, short int sequence, int voxelCount, VoxelDetail* voxelDetails, unsigned char*& bufferOut, int& sizeOut) { @@ -144,9 +133,9 @@ void VoxelEditPacketSender::queueVoxelEditMessages(PacketType type, int numberOf for (int i = 0; i < numberOfDetails; i++) { // use MAX_PACKET_SIZE since it's static and guarenteed to be larger than _maxPacketSize - static unsigned char bufferOut[MAX_PACKET_SIZE]; + unsigned char bufferOut[MAX_PACKET_SIZE]; int sizeOut = 0; - + if (encodeVoxelEditMessageDetails(type, 1, &details[i], &bufferOut[0], _maxPacketSize, sizeOut)) { queueOctreeEditMessage(type, bufferOut, sizeOut); } From 4bb7bb2b77ec1a08bb62df128da79f610cd9d077 Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Mon, 10 Feb 2014 12:48:45 -0800 Subject: [PATCH 2/2] make pending packets thread safe --- .../octree/src/OctreeEditPacketSender.cpp | 22 +++++++++---------- libraries/octree/src/OctreeEditPacketSender.h | 5 +++-- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/libraries/octree/src/OctreeEditPacketSender.cpp b/libraries/octree/src/OctreeEditPacketSender.cpp index 1c1ed13f8d..38abd15dc8 100644 --- a/libraries/octree/src/OctreeEditPacketSender.cpp +++ b/libraries/octree/src/OctreeEditPacketSender.cpp @@ -39,6 +39,7 @@ OctreeEditPacketSender::OctreeEditPacketSender() : } OctreeEditPacketSender::~OctreeEditPacketSender() { + _pendingPacketsLock.lock(); while (!_preServerSingleMessagePackets.empty()) { EditPacketBuffer* packet = _preServerSingleMessagePackets.front(); delete packet; @@ -49,6 +50,7 @@ OctreeEditPacketSender::~OctreeEditPacketSender() { delete packet; _preServerPackets.erase(_preServerPackets.begin()); } + _pendingPacketsLock.unlock(); //printf("OctreeEditPacketSender::~OctreeEditPacketSender() [%p] destroyed... \n", this); } @@ -115,6 +117,7 @@ void OctreeEditPacketSender::processPreServerExistsPackets() { assert(serversExist()); // we should only be here if we have jurisdictions // First send out all the single message packets... + _pendingPacketsLock.lock(); while (!_preServerSingleMessagePackets.empty()) { EditPacketBuffer* packet = _preServerSingleMessagePackets.front(); queuePacketToNodes(&packet->_currentBuffer[0], packet->_currentSize); @@ -129,6 +132,7 @@ void OctreeEditPacketSender::processPreServerExistsPackets() { delete packet; _preServerPackets.erase(_preServerPackets.begin()); } + _pendingPacketsLock.unlock(); // if while waiting for the jurisdictions the caller called releaseQueuedMessages() // then we want to honor that request now. @@ -140,8 +144,10 @@ void OctreeEditPacketSender::processPreServerExistsPackets() { void OctreeEditPacketSender::queuePendingPacketToNodes(PacketType type, unsigned char* buffer, ssize_t length) { // If we're asked to save messages while waiting for voxel servers to arrive, then do so... + if (_maxPendingMessages > 0) { EditPacketBuffer* packet = new EditPacketBuffer(type, buffer, length); + _pendingPacketsLock.lock(); _preServerSingleMessagePackets.push_back(packet); // if we've saved MORE than our max, then clear out the oldest packet... int allPendingMessages = _preServerSingleMessagePackets.size() + _preServerPackets.size(); @@ -150,6 +156,7 @@ void OctreeEditPacketSender::queuePendingPacketToNodes(PacketType type, unsigned delete packet; _preServerSingleMessagePackets.erase(_preServerSingleMessagePackets.begin()); } + _pendingPacketsLock.unlock(); } } @@ -197,6 +204,7 @@ void OctreeEditPacketSender::queueOctreeEditMessage(PacketType type, unsigned ch if (!serversExist()) { if (_maxPendingMessages > 0) { EditPacketBuffer* packet = new EditPacketBuffer(type, codeColorBuffer, length); + _pendingPacketsLock.lock(); _preServerPackets.push_back(packet); // if we've saved MORE than out max, then clear out the oldest packet... @@ -206,12 +214,11 @@ void OctreeEditPacketSender::queueOctreeEditMessage(PacketType type, unsigned ch delete packet; _preServerPackets.erase(_preServerPackets.begin()); } - } + _pendingPacketsLock.unlock(); + } return; // bail early } - //qDebug() << "queueOctreeEditMessage() line:" << __LINE__; - // We want to filter out edit messages for servers based on the server's Jurisdiction // But we can't really do that with a packed message, since each edit message could be destined // for a different server... So we need to actually manage multiple queued packets... one @@ -229,18 +236,14 @@ void OctreeEditPacketSender::queueOctreeEditMessage(PacketType type, unsigned ch if ((*_serverJurisdictions).find(nodeUUID) != (*_serverJurisdictions).end()) { const JurisdictionMap& map = (*_serverJurisdictions)[nodeUUID]; isMyJurisdiction = (map.isMyJurisdiction(codeColorBuffer, CHECK_NODE_ONLY) == JurisdictionMap::WITHIN); - //qDebug() << "queueOctreeEditMessage() line:" << __LINE__ << " isMyJurisdiction=" << isMyJurisdiction; } else { isMyJurisdiction = false; - //qDebug() << "queueOctreeEditMessage() line:" << __LINE__; } } if (isMyJurisdiction) { EditPacketBuffer& packetBuffer = _pendingEditPackets[nodeUUID]; packetBuffer._nodeUUID = nodeUUID; - //qDebug() << "queueOctreeEditMessage() line:" << __LINE__; - // If we're switching type, then we send the last one and start over if ((type != packetBuffer._currentType && packetBuffer._currentSize > 0) || (packetBuffer._currentSize + length >= _maxPacketSize)) { @@ -259,11 +262,8 @@ void OctreeEditPacketSender::queueOctreeEditMessage(PacketType type, unsigned ch // fixup the buffer for any clock skew if (node->getClockSkewUsec() != 0) { adjustEditPacketForClockSkew(codeColorBuffer, length, node->getClockSkewUsec()); - //qDebug() << "queueOctreeEditMessage() line:" << __LINE__; } - //qDebug() << "queueOctreeEditMessage() line:" << __LINE__; - memcpy(&packetBuffer._currentBuffer[packetBuffer._currentSize], codeColorBuffer, length); packetBuffer._currentSize += length; } @@ -280,14 +280,12 @@ void OctreeEditPacketSender::releaseQueuedMessages() { } else { for (std::map::iterator i = _pendingEditPackets.begin(); i != _pendingEditPackets.end(); i++) { releaseQueuedPacket(i->second); - //qDebug() << "releaseQueuedMessages() line:" << __LINE__; } } } void OctreeEditPacketSender::releaseQueuedPacket(EditPacketBuffer& packetBuffer) { if (packetBuffer._currentSize > 0 && packetBuffer._currentType != PacketTypeUnknown) { - //qDebug() << "OctreeEditPacketSender::releaseQueuedPacket() line:" << __LINE__; queuePacketToNode(packetBuffer._nodeUUID, &packetBuffer._currentBuffer[0], packetBuffer._currentSize); } packetBuffer._currentSize = 0; diff --git a/libraries/octree/src/OctreeEditPacketSender.h b/libraries/octree/src/OctreeEditPacketSender.h index 3f20ed45a3..75ad02a1c6 100644 --- a/libraries/octree/src/OctreeEditPacketSender.h +++ b/libraries/octree/src/OctreeEditPacketSender.h @@ -105,8 +105,9 @@ protected: // These are packets that are waiting to be processed because we don't yet know if there are servers int _maxPendingMessages; bool _releaseQueuedMessagesPending; - std::vector _preServerPackets; // these will get packed into other larger packets - std::vector _preServerSingleMessagePackets; // these will go out as is + QMutex _pendingPacketsLock; + QVector _preServerPackets; // these will get packed into other larger packets + QVector _preServerSingleMessagePackets; // these will go out as is NodeToJurisdictionMap* _serverJurisdictions;