Fixed some crashes

src/protocol/AcknowledgeManager.cpp

@@ -1,5 +1,5 @@
 #include "AcknowledgeManager.h"
-#include <math.h>
+#include <cmath>
 #include <misc/endianness.h>
 
 using namespace ts;
@@ -8,30 +8,27 @@ using namespace ts::protocol;
 using namespace std;
 using namespace std::chrono;
 
-AcknowledgeManager::AcknowledgeManager() {}
+AcknowledgeManager::AcknowledgeManager() = default;
 
 AcknowledgeManager::~AcknowledgeManager() {
-    {
+    this->reset();
-        lock_guard<recursive_mutex> lock(this->entry_lock);
-        for(const auto& entry : this->entries)
-            if(entry->acknowledge_listener)
-                entry->acknowledge_listener->executionFailed("deleted");
-        this->entries.clear();
-    }
 }
 
 void AcknowledgeManager::reset() {
     {
-        lock_guard<recursive_mutex> lock(this->entry_lock);
+        std::unique_lock lock{this->entry_lock};
-        for(const auto& entry : this->entries)
+        auto pending_entries = std::move(this->entries);
+        lock.unlock();
+
+        /* save because entries are not accessable anymore */
+        for(const auto& entry : pending_entries)
             if(entry->acknowledge_listener)
                 entry->acknowledge_listener->executionFailed("reset");
-        this->entries.clear();
     }
 }
 
 size_t AcknowledgeManager::awaiting_acknowledge() {
-    lock_guard<recursive_mutex> lock(this->entry_lock);
+    std::lock_guard lock(this->entry_lock);
     return this->entries.size();
 }
 
@@ -54,7 +51,7 @@ void AcknowledgeManager::process_packet(ts::protocol::BasicPacket &packet) {
     entry->acknowledged = false;
     entry->send_count = 1;
     {
-        lock_guard<recursive_mutex> lock(this->entry_lock);
+        std::lock_guard lock(this->entry_lock);
         this->entries.push_front(std::move(entry));
     }
 }
@@ -62,25 +59,18 @@ void AcknowledgeManager::process_packet(ts::protocol::BasicPacket &packet) {
 bool AcknowledgeManager::process_acknowledge(uint8_t packet_type, const pipes::buffer_view& payload, std::string& error) {
     if(payload.length() < 2) return false;
 
-    PacketType target_type = PacketType::UNDEFINED;
+    PacketType target_type{packet_type == protocol::ACK_LOW ? PacketType::COMMAND_LOW : PacketType::COMMAND};
-    uint16_t target_id = 0;
+    uint16_t target_id{be2le16((char*) payload.data_ptr())};
-
-    if(packet_type == protocol::ACK_LOW) target_type = PacketType::COMMAND_LOW;
-    else if(packet_type == protocol::ACK) target_type = PacketType::COMMAND;
-    target_id = be2le16((char*) payload.data_ptr());
-    //debugMessage(0, "Got ack for {} {}", target_type, target_id);
-
-    if(target_type == PacketType::UNDEFINED) {
-        error = "Invalid packet type (" + to_string(target_type) + ")";
-        return false;
-    }
 
     std::shared_ptr<Entry> entry;
+    std::unique_ptr<threads::Future<bool>> ack_listener;
     {
-        lock_guard<recursive_mutex> lock(this->entry_lock);
+        std::lock_guard lock{this->entry_lock};
         for(auto it = this->entries.begin(); it != this->entries.end(); it++) {
             if((*it)->packet_type == target_type && (*it)->packet_id == target_id) {
                 entry = *it;
+                ack_listener = std::move(entry->acknowledge_listener); /* move it out so nobody else could call it as well */
+
                 entry->send_count--;
                 if(entry->send_count == 0)
                     this->entries.erase(it);
@@ -100,46 +90,44 @@ bool AcknowledgeManager::process_acknowledge(uint8_t packet_type, const pipes::b
     }
 
     entry->acknowledged = true;
-    if(entry->acknowledge_listener)
+    if(ack_listener) ack_listener->executionSucceed(true);
-        entry->acknowledge_listener->executionSucceed(true);
-    entry->acknowledge_listener.reset();
-
     return true;
 }
 
 ssize_t AcknowledgeManager::execute_resend(const system_clock::time_point& now , std::chrono::system_clock::time_point &next_resend,std::deque<pipes::buffer>& buffers, string& error) {
-    ssize_t resend_count = 0;
+    size_t resend_count{0};
 
-    deque<shared_ptr<Entry>> need_resend;
+    vector<shared_ptr<Entry>> need_resend;
     {
-        deque<shared_ptr<Entry>> erase;
+        bool cleanup{false};
+        std::lock_guard lock{this->entry_lock};
+        need_resend.resize(this->entries.size());
 
-        lock_guard<recursive_mutex> lock(this->entry_lock);
         for (auto &entry : this->entries) {
-            if(!entry->acknowledged && entry->next_resend <= now) {
+            if(entry->acknowledged) {
-                entry->resend_period = entry->resend_period + milliseconds((int) ceil(this->average_response * 2));
+                if(entry->next_resend + entry->resend_period <= now) { // Some resends are lost. So we just drop it after time
+                    entry.reset();
+                    cleanup = true;
+                }
+            } else {
+               if(entry->next_resend <= now) {
+                   entry->resend_period = entry->resend_period + milliseconds{(int) ceil(this->average_response * 2)};
                    if(entry->resend_period.count() > 1000)
                        entry->resend_period = milliseconds(1000);
                    else if(entry->resend_period.count() < 25)
                        entry->resend_period = milliseconds(25);
 
                    entry->next_resend = now + entry->resend_period;
-                need_resend.push_front(entry);
+                   need_resend.push_back(entry);
                }
-            if(entry->acknowledged) {
-                if(entry->next_resend + entry->resend_period <= now) { //Timeout for may (more acknowledges)
-                    erase.push_back(entry);
-                }
-            } else {
                 if(next_resend > entry->next_resend)
                     next_resend = entry->next_resend;
             }
         }
 
-        for(const auto& e : erase) {
+        if(cleanup) {
-            auto it = find(this->entries.begin(), this->entries.end(), e);
+            this->entries.erase(std::remove_if(this->entries.begin(), this->entries.end(),
-            if(it != this->entries.end())
+                    [](const auto& entry) { return !entry; }), this->entries.end());
-                this->entries.erase(it);
         }
     }
 

src/protocol/AcknowledgeManager.h

@@ -4,8 +4,7 @@
 #include <protocol/Packet.h>
 
 #define DEBUG_ACKNOWLEDGE
-namespace ts {
+namespace ts::connection {
-    namespace connection {
     class VoiceClientConnection;
     class AcknowledgeManager {
         struct Entry {
@@ -40,7 +39,7 @@ namespace ts {
                     std::string& /* error */
             );
         private:
-                std::recursive_mutex entry_lock;
+            std::mutex entry_lock;
             std::deque<std::shared_ptr<Entry>> entries;
 
             std::chrono::milliseconds resend_delay{500};
@@ -48,4 +47,3 @@ namespace ts {
             double average_response = 20;
     };
 }
-}