Fix unnecessary QoS packet creation

The extraneous creation would happen when multiple subscribes at different QoS levels happen.

Fix unnecessary QoS packet creation
The extraneous creation would happen when multiple subscribes at different QoS levels happen.
Wiebe Cazemier
1 parent 4a16d48b
Showing 3 changed files with 10 additions and 9 deletions
client.cpp
publishcopyfactory.cpp
publishcopyfactory.h
@@ -251,14 +251,14 @@ void Client::writeMqttPacketAndBlameThisClient(PublishCopyFactory &amp;copyFactory, 
     MqttPacket *p = copyFactory.getOptimumPacket(max_qos, this->protocolVersion, topic_alias, skip_topic);
-    assert(p->getQos() <= max_qos);
+    assert(static_cast<bool>(p->getQos()) == static_cast<bool>(max_qos));
     if (p->getQos() > 0)
     {
         // This may change the packet ID and QoS of the incoming packet for each subscriber, but because we don't store that packet anywhere,
         // that should be fine.
         p->setPacketId(packet_id);
-        p->setQos(max_qos);
+        p->setQos(copyFactory.getEffectiveQos(max_qos));
     }
     writeMqttPacketAndBlameThisClient(*p);
@@ -19,31 +19,32 @@ PublishCopyFactory::PublishCopyFactory(Publish *publish) :
 MqttPacket *PublishCopyFactory::getOptimumPacket(const char max_qos, const ProtocolVersion protocolVersion, uint16_t topic_alias, bool skip_topic)
 {
+    const char actualQos = getEffectiveQos(max_qos);
+
     if (packet)
     {
         if (protocolVersion >= ProtocolVersion::Mqtt5 && (packet->containsClientSpecificProperties() || topic_alias > 0))
         {
             Publish newPublish(packet->getPublishData());
-            newPublish.qos = max_qos;
+            newPublish.qos = actualQos;
             newPublish.topicAlias = topic_alias;
             newPublish.skipTopic = skip_topic;
             this->oneShotPacket = std::make_unique<MqttPacket>(protocolVersion, newPublish);
             return this->oneShotPacket.get();
         }
-        if (packet->getProtocolVersion() == protocolVersion && orgQos == max_qos)
+        if (packet->getProtocolVersion() == protocolVersion && static_cast<bool>(orgQos) == static_cast<bool>(actualQos))
         {
-            assert(orgQos == packet->getQos());
             return packet;
         }
-        const int cache_key = (static_cast<uint8_t>(protocolVersion) * 10) + max_qos;
+        const int cache_key = (static_cast<uint8_t>(protocolVersion) * 10) + actualQos;
         std::unique_ptr<MqttPacket> &cachedPack = constructedPacketCache[cache_key];
         if (!cachedPack)
         {
             Publish newPublish(packet->getPublishData());
-            newPublish.qos = max_qos;
+            newPublish.qos = actualQos;
             cachedPack = std::make_unique<MqttPacket>(protocolVersion, newPublish);
         }
@@ -53,7 +54,7 @@ MqttPacket *PublishCopyFactory::getOptimumPacket(const char max_qos, const Proto
     // Getting an instance of a Publish object happens at least on retained messages, will messages and SYS topics. It's low traffic, anyway.
     assert(publish);
-    publish->qos = getEffectiveQos(max_qos);
+    publish->qos = actualQos;
     this->oneShotPacket = std::make_unique<MqttPacket>(protocolVersion, *publish);
     return this->oneShotPacket.get();
@@ -11,7 +11,7 @@
  * @brief The PublishCopyFactory class is for managing copies of an incoming publish, including sometimes not making copies at all.
  *
  * The idea is that certain incoming packets can just be written to the receiving client as-is, without constructing a new one. We do have to change the bytes
- * where the QoS is stored, so we keep track of the original.
+ * where the QoS is stored, so we keep track of the original QoS.
  *
  * Ownership info: object of this type are never copied or transferred, so the internal pointers come from (near) the same scope these objects
  * are created from.