Implement unique Command ID to replace void* usage

Ran into a nasty segfault when using 100 parallel asynchronous command loops, where a command's address would match up with a previous command's address. To circumvent the whole issue, Redox keeps a counter of how many Commands it has created and assigns each Command a unique id in the order they are created. Changed all data structures and methods to pass around Command IDs, so nothing uses void* anymore. I consider this a very good thing, but it did seem to slow down performance 20-30% because of the extra lookups. Will see if there's a solution later.

Implement unique Command ID to replace void* usage
Ran into a nasty segfault when using 100 parallel asynchronous command loops, where a command's address would match up with a previous command's address. To circumvent the whole issue, Redox keeps a counter of how many Commands it has created and assigns each Command a unique id in the order they are created. Changed all data structures and methods to pass around Command IDs, so nothing uses void* anymore. I consider this a very good thing, but it did seem to slow down performance 20-30% because of the extra lookups. Will see if there's a solution later.
Hayk Martirosyan
1 parent c2436e64
Showing 3 changed files with 103 additions and 52 deletions
src/command.hpp
src/redox.cpp
src/redox.hpp
@@ -38,6 +38,7 @@ friend void submit_command_callback&lt;ReplyT&gt;(struct ev_loop* loop, ev_timer* time
 public:
   Command(
     Redox* rdx,
+    long id,
     const std::string& cmd,
     const std::function<void(const std::string&, const ReplyT&)>& callback,
     const std::function<void(const std::string&, int status)>& error_callback,
@@ -47,6 +48,7 @@ public:
   Redox* rdx;
+  const long id;
   const std::string cmd;
   const double repeat;
   const double after;
@@ -106,11 +108,12 @@ private:
 template<class ReplyT>
 Command<ReplyT>::Command(
     Redox* rdx,
+    long id,
     const std::string& cmd,
     const std::function<void(const std::string&, const ReplyT&)>& callback,
     const std::function<void(const std::string&, int status)>& error_callback,
     double repeat, double after, bool free_memory
-) : rdx(rdx), cmd(cmd), repeat(repeat), after(after), free_memory(free_memory),
+) : rdx(rdx), id(id), cmd(cmd), repeat(repeat), after(after), free_memory(free_memory),
     callback(callback), error_callback(error_callback)
 {
   timer_guard.lock();
@@ -119,6 +122,11 @@ Command&lt;ReplyT&gt;::Command(
 template<class ReplyT>
 void Command<ReplyT>::process_reply() {
+  if(cmd == "GET simple_loop:count") {
+    std::cout << "In process_reply, cmd = " << cmd << ", reply_obj = " << reply_obj << std::endl;
+    std::cout << "reply int: " << reply_obj->integer << std::endl;
+    std::cout << "reply str: " << reply_obj->str << std::endl;
+  }
   free_guard.lock();
   invoke_callback();
@@ -126,11 +134,13 @@ void Command&lt;ReplyT&gt;::process_reply() {
   pending--;
   if(!free_memory) {
+    std::cout << "Command memory not being freed, free_memory = " << free_memory << std::endl;
     // Allow free() method to free memory
     free_guard.unlock();
     return;
   }
+
   free_reply_object();
   if((pending == 0) && (repeat == 0)) {
@@ -165,7 +175,7 @@ void Command&lt;ReplyT&gt;::free_reply_object() {
 template<class ReplyT>
 void Command<ReplyT>::free_command(Command<ReplyT>* c) {
   c->rdx->commands_deleted += 1;
-  c->rdx->remove_active_command(c);
+  c->rdx->template remove_active_command<ReplyT>(c->id);
 //  std::cout << "[INFO] Deleted Command " << c->rdx->commands_created << " at " << c << std::endl;
   delete c;
 }
@@ -147,16 +147,34 @@ void Redox::stop() {
 template<class ReplyT>
 void command_callback(redisAsyncContext *ctx, void *r, void *privdata) {
-  Command<ReplyT>* c = (Command<ReplyT>*) privdata;
-  redisReply* reply_obj = (redisReply*) r;
   Redox* rdx = (Redox*) ctx->data;
+  long id = (long)privdata;
+  redisReply* reply_obj = (redisReply*) r;
-  if(!rdx->is_active_command(c)) {
+  auto& command_map = rdx->get_command_map<ReplyT>();
+  auto it = command_map.find(id);
+  if(it == command_map.end()) {
+    cout << "[ERROR] Couldn't find Command " << id << " in command_map." << endl;
+    freeReplyObject(r);
+    return;
+  };
+  Command<ReplyT>* c = it->second;
+
+  if(!rdx->is_active_command(c->id)) {
     std::cout << "[INFO] Ignoring callback, command " << c << " was freed." << std::endl;
     freeReplyObject(r);
     return;
   }
+  if(c->cmd == "GET simple_loop:count") {
+    std::cout << "In command_callback = " << c->cmd << " at " << c << ", reply_obj = " << reply_obj << std::endl;
+    std::cout << "reply type: " << reply_obj->type << std::endl;
+    std::cout << "reply int: " << reply_obj->integer << std::endl;
+    std::cout << "reply str: " << reply_obj->str << std::endl;
+//    std::string s(reply_obj->str);
+//    std::cout << "string object: " << s << std::endl;
+  }
+
   c->reply_obj = reply_obj;
   c->process_reply();
@@ -170,8 +188,11 @@ void command_callback(redisAsyncContext *ctx, void *r, void *privdata) {
 */
 template<class ReplyT>
 bool submit_to_server(Command<ReplyT>* c) {
+  if(c->cmd == "GET simple_loop:count") {
+    std::cout << "submit_to_server for cmd at " << c << ": " << c->cmd << std::endl;
+  }
   c->pending++;
-  if (redisAsyncCommand(c->rdx->ctx, command_callback<ReplyT>, (void*)c, c->cmd.c_str()) != REDIS_OK) {
+  if (redisAsyncCommand(c->rdx->ctx, command_callback<ReplyT>, (void*)c->id, c->cmd.c_str()) != REDIS_OK) {
     cerr << "[ERROR] Could not send \"" << c->cmd << "\": " << c->rdx->ctx->errstr << endl;
     c->invoke_error(REDOX_SEND_ERROR);
     return false;
@@ -182,7 +203,16 @@ bool submit_to_server(Command&lt;ReplyT&gt;* c) {
 template<class ReplyT>
 void submit_command_callback(struct ev_loop* loop, ev_timer* timer, int revents) {
-  auto c = (Command<ReplyT>*)timer->data;
+  Redox* rdx = (Redox*) ev_userdata(loop);
+  long id = (long)timer->data;
+
+  auto& command_map = rdx->get_command_map<ReplyT>();
+  auto it = command_map.find(id);
+  if(it == command_map.end()) {
+    cout << "[ERROR] Couldn't find Command " << id << " in command_map." << endl;
+    return;
+  };
+  Command<ReplyT>* c = it->second;
   if(c->is_completed()) {
@@ -202,20 +232,23 @@ void submit_command_callback(struct ev_loop* loop, ev_timer* timer, int revents)
 }
 template<class ReplyT>
-bool Redox::process_queued_command(void* c_ptr) {
+bool Redox::process_queued_command(long id) {
   auto& command_map = get_command_map<ReplyT>();
-  auto it = command_map.find(c_ptr);
+  auto it = command_map.find(id);
   if(it == command_map.end()) return false;
   Command<ReplyT>* c = it->second;
-  command_map.erase(c_ptr);
+
+  if(c->cmd == "GET simple_loop:count") {
+    std::cout << "process_queued_command for cmd at " << c << ": " << c->cmd << std::endl;
+  }
   if((c->repeat == 0) && (c->after == 0)) {
     submit_to_server<ReplyT>(c);
   } else {
-    c->timer.data = (void*)c;
+    c->timer.data = (void*)c->id;
     ev_timer_init(&c->timer, submit_command_callback<ReplyT>, c->after, c->repeat);
     ev_timer_start(evloop, &c->timer);
@@ -232,13 +265,13 @@ void Redox::process_queued_commands() {
   while(!command_queue.empty()) {
-    void* c_ptr = command_queue.front();
-    if(process_queued_command<redisReply*>(c_ptr)) {}
-    else if(process_queued_command<string>(c_ptr)) {}
-    else if(process_queued_command<char*>(c_ptr)) {}
-    else if(process_queued_command<int>(c_ptr)) {}
-    else if(process_queued_command<long long int>(c_ptr)) {}
-    else if(process_queued_command<nullptr_t>(c_ptr)) {}
+    long id = command_queue.front();
+    if(process_queued_command<redisReply*>(id)) {}
+    else if(process_queued_command<string>(id)) {}
+    else if(process_queued_command<char*>(id)) {}
+    else if(process_queued_command<int>(id)) {}
+    else if(process_queued_command<long long int>(id)) {}
+    else if(process_queued_command<nullptr_t>(id)) {}
     else throw runtime_error("[FATAL] Command pointer not found in any queue!");
     command_queue.pop();
@@ -247,22 +280,22 @@ void Redox::process_queued_commands() {
 // ----------------------------
-template<> unordered_map<void*, Command<redisReply*>*>&
+template<> unordered_map<long, Command<redisReply*>*>&
 Redox::get_command_map() { return commands_redis_reply; }
-template<> unordered_map<void*, Command<string>*>&
+template<> unordered_map<long, Command<string>*>&
 Redox::get_command_map() { return commands_string_r; }
-template<> unordered_map<void*, Command<char*>*>&
+template<> unordered_map<long, Command<char*>*>&
 Redox::get_command_map() { return commands_char_p; }
-template<> unordered_map<void*, Command<int>*>&
+template<> unordered_map<long, Command<int>*>&
 Redox::get_command_map() { return commands_int; }
-template<> unordered_map<void*, Command<long long int>*>&
+template<> unordered_map<long, Command<long long int>*>&
 Redox::get_command_map() { return commands_long_long_int; }
-template<> unordered_map<void*, Command<nullptr_t>*>&
+template<> unordered_map<long, Command<nullptr_t>*>&
 Redox::get_command_map() { return commands_null; }
 // ----------------------------
@@ -70,17 +70,22 @@ public:
 //  void subscribe(std::string channel, std::function<void(std::string channel, std::string msg)> callback);
 //  void unsubscribe(std::string channel);
-  std::atomic_int commands_created = {0};
-  std::atomic_int commands_deleted = {0};
+  std::atomic_long commands_created = {0};
+  std::atomic_long commands_deleted = {0};
-  bool is_active_command(void* c_ptr) {
-    return active_commands.find(c_ptr) != active_commands.end();
+  bool is_active_command(const long id) {
+    return active_commands.find(id) != active_commands.end();
   }
-  void remove_active_command(void* c_ptr) {
-    active_commands.erase(c_ptr);
+  template<class ReplyT>
+  void remove_active_command(const long id) {
+    active_commands.erase(id);
+    get_command_map<ReplyT>().erase(id);
   }
+  template<class ReplyT>
+  std::unordered_map<long, Command<ReplyT>*>& get_command_map();
+
 private:
   // Redox server
@@ -103,25 +108,22 @@ private:
   std::thread event_loop_thread;
-  std::unordered_map<void*, Command<redisReply*>*> commands_redis_reply;
-  std::unordered_map<void*, Command<std::string>*> commands_string_r;
-  std::unordered_map<void*, Command<char*>*> commands_char_p;
-  std::unordered_map<void*, Command<int>*> commands_int;
-  std::unordered_map<void*, Command<long long int>*> commands_long_long_int;
-  std::unordered_map<void*, Command<std::nullptr_t>*> commands_null;
-
-  template<class ReplyT>
-  std::unordered_map<void*, Command<ReplyT>*>& get_command_map();
+  std::unordered_map<long, Command<redisReply*>*> commands_redis_reply;
+  std::unordered_map<long, Command<std::string>*> commands_string_r;
+  std::unordered_map<long, Command<char*>*> commands_char_p;
+  std::unordered_map<long, Command<int>*> commands_int;
+  std::unordered_map<long, Command<long long int>*> commands_long_long_int;
+  std::unordered_map<long, Command<std::nullptr_t>*> commands_null;
-  std::queue<void*> command_queue;
+  std::queue<long> command_queue;
   std::mutex queue_guard;
   void process_queued_commands();
   template<class ReplyT>
-  bool process_queued_command(void* cmd_ptr);
+  bool process_queued_command(long id);
-  // Commands created but not yet deleted
-  std::unordered_set<void*> active_commands;
+  // Commands created but not yet deleted (stored by id)
+  std::unordered_set<long> active_commands;
 };
 // ---------------------------
@@ -136,13 +138,18 @@ Command&lt;ReplyT&gt;* Redox::command(
   bool free_memory
 ) {
   std::lock_guard<std::mutex> lg(queue_guard);
-  auto* c = new Command<ReplyT>(this, cmd, callback, error_callback, repeat, after, free_memory);
-  void* c_ptr = (void*)c;
-  get_command_map<ReplyT>()[c_ptr] = c;
-  command_queue.push(c_ptr);
-  active_commands.insert(c_ptr);
+
   commands_created += 1;
-//  std::cout << "[DEBUG] Created Command " << commands_created << " at " << c << std::endl;
+  auto* c = new Command<ReplyT>(this, commands_created, cmd,
+    callback, error_callback, repeat, after, free_memory);
+
+  get_command_map<ReplyT>()[c->id] = c;
+  active_commands.insert(c->id);
+  command_queue.push(c->id);
+  std::cout << "[DEBUG] Created Command " << c->id << " at " << c << std::endl;
+  if(cmd == "GET simple_loop:count") {
+    std::cout << "Command created at " << c << ": " << c->cmd << std::endl;
+  }
   return c;
 }
@@ -154,7 +161,7 @@ bool Redox::cancel(Command&lt;ReplyT&gt;* c) {
     return false;
   }
-  std::cout << "[INFO] Canceling command at " << c << std::endl;
+  std::cout << "[INFO] Canceling command " << c->id << " at " << c << std::endl;
   c->completed = true;
   return true;
@@ -174,6 +181,7 @@ Command&lt;ReplyT&gt;* Redox::command_blocking(const std::string&amp; cmd) {
   Command<ReplyT>* c = command<ReplyT>(cmd,
     [&val, &status, &m, &cv](const std::string& cmd_str, const ReplyT& reply) {
       std::unique_lock<std::mutex> ul(m);
+      std::cout << "success callback: " << cmd_str << std::endl;
       val = reply;
       status = REDOX_OK;
       ul.unlock();
@@ -187,10 +195,10 @@ Command&lt;ReplyT&gt;* Redox::command_blocking(const std::string&amp; cmd) {
     },
     0, 0, false // No repeats, don't free memory
   );
-
+  std::cout << "command blocking cv wait starting" << std::endl;
   // Wait until a callback is invoked
   cv.wait(lk, [&status] { return status != REDOX_UNINIT; });
-
+  std::cout << "command blocking cv wait over" << std::endl;
   c->reply_val = val;
   c->reply_status = status;