From 8fb3015ec8677b71f8315f62ec83e0501c18d6ff Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Mon, 5 Mar 2018 16:51:05 -0800 Subject: [PATCH 1/3] Fix for missing avatar on second replay Fixes a logic problem in the handshake request handling; deals with KillAvatar from the Agent by removing the node-linked data, not the actual Node; tweaks some of the network debug output. --- assignment-client/src/avatars/AvatarMixer.cpp | 4 +++- libraries/networking/src/udt/Connection.cpp | 3 ++- libraries/networking/src/udt/Socket.cpp | 4 ++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 3ca924b007..dd7db5a21d 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -605,7 +605,9 @@ void AvatarMixer::handleAvatarIdentityPacket(QSharedPointer mes void AvatarMixer::handleKillAvatarPacket(QSharedPointer message, SharedNodePointer node) { auto start = usecTimestampNow(); - DependencyManager::get()->processKillNode(*message); + node->stopPingTimer(); + emit(DependencyManager::get()->nodeKilled(node)); + node->setLinkedData(nullptr); auto end = usecTimestampNow(); _handleKillAvatarPacketElapsedTime += (end - start); diff --git a/libraries/networking/src/udt/Connection.cpp b/libraries/networking/src/udt/Connection.cpp index 77ed589e0b..c1fe6ccd85 100644 --- a/libraries/networking/src/udt/Connection.cpp +++ b/libraries/networking/src/udt/Connection.cpp @@ -576,8 +576,9 @@ void Connection::processControl(ControlPacketPointer controlPacket) { // where the other end expired our connection. Let's reset. #ifdef UDT_CONNECTION_DEBUG - qCDebug(networking) << "Got handshake request, stopping SendQueue"; + qCDebug(networking) << "Got HandshakeRequest from" << _destination << ", stopping SendQueue"; #endif + _hasReceivedHandshakeACK = false; stopSendQueue(); } break; diff --git a/libraries/networking/src/udt/Socket.cpp b/libraries/networking/src/udt/Socket.cpp index 8b93a05130..5324946f82 100644 --- a/libraries/networking/src/udt/Socket.cpp +++ b/libraries/networking/src/udt/Socket.cpp @@ -402,6 +402,10 @@ void Socket::readPendingDatagrams() { packet->getDataSize(), packet->getPayloadSize())) { // the connection could not be created or indicated that we should not continue processing this packet +#ifdef UDT_CONNECTION_DEBUG + qCDebug(networking) << "Bad packet version" << (unsigned int)NLPacket::versionInHeader(*packet) + << ", type" << NLPacket::typeInHeader(*packet); +#endif continue; } } From 547d86a2d52699d27b0cb553e0e47ecd02a65f2d Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Mon, 5 Mar 2018 18:10:15 -0800 Subject: [PATCH 2/3] Improve debugging diagnostic --- libraries/networking/src/udt/Socket.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/src/udt/Socket.cpp b/libraries/networking/src/udt/Socket.cpp index 5324946f82..4189cb613c 100644 --- a/libraries/networking/src/udt/Socket.cpp +++ b/libraries/networking/src/udt/Socket.cpp @@ -403,7 +403,7 @@ void Socket::readPendingDatagrams() { packet->getPayloadSize())) { // the connection could not be created or indicated that we should not continue processing this packet #ifdef UDT_CONNECTION_DEBUG - qCDebug(networking) << "Bad packet version" << (unsigned int)NLPacket::versionInHeader(*packet) + qCDebug(networking) << "Can't process packet: version" << (unsigned int)NLPacket::versionInHeader(*packet) << ", type" << NLPacket::typeInHeader(*packet); #endif continue; From 30206ff33e2a4275cc0f4e82029f52c4d341929b Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 6 Mar 2018 12:28:55 -0800 Subject: [PATCH 3/3] Call killed-avatar slot directly rather than by signal Rename nodeKilled slot to handleKilledAvatar since the node isn't killed in this case and call directly from handleKillAvatarPacket(). --- assignment-client/src/avatars/AvatarMixer.cpp | 27 ++++++++++--------- assignment-client/src/avatars/AvatarMixer.h | 2 +- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index dd7db5a21d..929941c05c 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -42,7 +42,7 @@ AvatarMixer::AvatarMixer(ReceivedMessage& message) : ThreadedAssignment(message) { // make sure we hear about node kills so we can tell the other nodes - connect(DependencyManager::get().data(), &NodeList::nodeKilled, this, &AvatarMixer::nodeKilled); + connect(DependencyManager::get().data(), &NodeList::nodeKilled, this, &AvatarMixer::handleAvatarKilled); auto& packetReceiver = DependencyManager::get()->getPacketReceiver(); packetReceiver.registerListener(PacketType::AvatarData, this, "queueIncomingPacket"); @@ -423,14 +423,15 @@ void AvatarMixer::throttle(std::chrono::microseconds duration, int frame) { } } -void AvatarMixer::nodeKilled(SharedNodePointer killedNode) { - if (killedNode->getType() == NodeType::Agent - && killedNode->getLinkedData()) { + +void AvatarMixer::handleAvatarKilled(SharedNodePointer avatarNode) { + if (avatarNode->getType() == NodeType::Agent + && avatarNode->getLinkedData()) { auto nodeList = DependencyManager::get(); { // decrement sessionDisplayNames table and possibly remove - QMutexLocker nodeDataLocker(&killedNode->getLinkedData()->getMutex()); - AvatarMixerClientData* nodeData = dynamic_cast(killedNode->getLinkedData()); + QMutexLocker nodeDataLocker(&avatarNode->getLinkedData()->getMutex()); + AvatarMixerClientData* nodeData = dynamic_cast(avatarNode->getLinkedData()); const QString& baseDisplayName = nodeData->getBaseDisplayName(); // No sense guarding against very rare case of a node with no entry, as this will work without the guard and do one less lookup in the common case. if (--_sessionDisplayNames[baseDisplayName].second <= 0) { @@ -447,12 +448,12 @@ void AvatarMixer::nodeKilled(SharedNodePointer killedNode) { // we relay avatar kill packets to agents that are not upstream // and downstream avatar mixers, if the node that was just killed was being replicated return (node->getType() == NodeType::Agent && !node->isUpstream()) || - (killedNode->isReplicated() && shouldReplicateTo(*killedNode, *node)); + (avatarNode->isReplicated() && shouldReplicateTo(*avatarNode, *node)); }, [&](const SharedNodePointer& node) { if (node->getType() == NodeType::Agent) { if (!killPacket) { killPacket = NLPacket::create(PacketType::KillAvatar, NUM_BYTES_RFC4122_UUID + sizeof(KillAvatarReason)); - killPacket->write(killedNode->getUUID().toRfc4122()); + killPacket->write(avatarNode->getUUID().toRfc4122()); killPacket->writePrimitive(KillAvatarReason::AvatarDisconnected); } @@ -462,7 +463,7 @@ void AvatarMixer::nodeKilled(SharedNodePointer killedNode) { if (!replicatedKillPacket) { replicatedKillPacket = NLPacket::create(PacketType::ReplicatedKillAvatar, NUM_BYTES_RFC4122_UUID + sizeof(KillAvatarReason)); - replicatedKillPacket->write(killedNode->getUUID().toRfc4122()); + replicatedKillPacket->write(avatarNode->getUUID().toRfc4122()); replicatedKillPacket->writePrimitive(KillAvatarReason::AvatarDisconnected); } @@ -479,7 +480,7 @@ void AvatarMixer::nodeKilled(SharedNodePointer killedNode) { return false; } - if (node->getUUID() == killedNode->getUUID()) { + if (node->getUUID() == avatarNode->getUUID()) { return false; } @@ -489,7 +490,7 @@ void AvatarMixer::nodeKilled(SharedNodePointer killedNode) { QMetaObject::invokeMethod(node->getLinkedData(), "cleanupKilledNode", Qt::AutoConnection, - Q_ARG(const QUuid&, QUuid(killedNode->getUUID()))); + Q_ARG(const QUuid&, QUuid(avatarNode->getUUID()))); } ); } @@ -605,8 +606,8 @@ void AvatarMixer::handleAvatarIdentityPacket(QSharedPointer mes void AvatarMixer::handleKillAvatarPacket(QSharedPointer message, SharedNodePointer node) { auto start = usecTimestampNow(); - node->stopPingTimer(); - emit(DependencyManager::get()->nodeKilled(node)); + handleAvatarKilled(node); + node->setLinkedData(nullptr); auto end = usecTimestampNow(); _handleKillAvatarPacketElapsedTime += (end - start); diff --git a/assignment-client/src/avatars/AvatarMixer.h b/assignment-client/src/avatars/AvatarMixer.h index cb5f536faa..1fbfd7338b 100644 --- a/assignment-client/src/avatars/AvatarMixer.h +++ b/assignment-client/src/avatars/AvatarMixer.h @@ -39,7 +39,7 @@ public slots: /// runs the avatar mixer void run() override; - void nodeKilled(SharedNodePointer killedNode); + void handleAvatarKilled(SharedNodePointer killedNode); void sendStatsPacket() override;