From e2574666f604e1a281e5565f53d0c7e2650bc610 Mon Sep 17 00:00:00 2001
From: Simon <simon@bitcartel.com>
Date: Thu, 8 Sep 2016 12:32:29 -0700
Subject: [PATCH] Add lock for member variables. Clean up and clarify that id_
 and creation_time_ are never to be mutated anywhere. Fix incomplete
 copy/assignment constructors.

---
 src/asyncrpcoperation.cpp | 24 +++++++++++++++++-------
 src/asyncrpcoperation.h   | 19 +++++++++++--------
 2 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/src/asyncrpcoperation.cpp b/src/asyncrpcoperation.cpp
index 9e4595c2..508cd0a5 100644
--- a/src/asyncrpcoperation.cpp
+++ b/src/asyncrpcoperation.cpp
@@ -31,22 +31,28 @@ std::map<OperationStatus, std::string> OperationStatusMap = {
 AsyncRPCOperation::AsyncRPCOperation() : error_code_(0), error_message_() {
     // Set a unique reference for each operation
     boost::uuids::uuid uuid = uuidgen();
-    std::string s = "opid-" + boost::uuids::to_string(uuid);
-    set_id(s);
-    
-    set_state(OperationStatus::READY);
+    id_ = "opid-" + boost::uuids::to_string(uuid);
     creation_time_ = (int64_t)time(NULL);
+    set_state(OperationStatus::READY);
 }
 
 AsyncRPCOperation::AsyncRPCOperation(const AsyncRPCOperation& o) :
-    id_(o.id_), creation_time_(o.creation_time_), state_(o.state_.load())
+        id_(o.id_), creation_time_(o.creation_time_), state_(o.state_.load()),
+        start_time_(o.start_time_), end_time_(o.end_time_),
+        error_code_(o.error_code_), error_message_(o.error_message_),
+        result_(o.result_)
 {
 }
 
 AsyncRPCOperation& AsyncRPCOperation::operator=( const AsyncRPCOperation& other ) {
-    this->id_ = other.getId();
+    this->id_ = other.id_;
     this->creation_time_ = other.creation_time_;
     this->state_.store(other.state_.load());
+    this->start_time_ = other.start_time_;
+    this->end_time_ = other.end_time_;
+    this->error_code_ = other.error_code_;
+    this->error_message_ = other.error_message_;
+    this->result_ = other.result_;
     return *this;
 }
 
@@ -67,6 +73,7 @@ void AsyncRPCOperation::cancel() {
  * Start timing the execution run of the code you're interested in
  */
 void AsyncRPCOperation::start_execution_clock() {
+    std::lock_guard<std::mutex> guard(lock_);
     start_time_ = std::chrono::system_clock::now();
 }
 
@@ -74,6 +81,7 @@ void AsyncRPCOperation::start_execution_clock() {
  * Stop timing the execution run
  */
 void AsyncRPCOperation::stop_execution_clock() {
+    std::lock_guard<std::mutex> guard(lock_);
     end_time_ = std::chrono::system_clock::now();
 }
 
@@ -115,6 +123,7 @@ Value AsyncRPCOperation::getError() const {
         return Value::null;
     }
 
+    std::lock_guard<std::mutex> guard(lock_);
     Object error;
     error.push_back(Pair("code", this->error_code_));
     error.push_back(Pair("message", this->error_message_));
@@ -130,6 +139,7 @@ Value AsyncRPCOperation::getResult() const {
         return Value::null;
     }
 
+    std::lock_guard<std::mutex> guard(lock_);
     return this->result_;
 }
 
@@ -143,7 +153,7 @@ Value AsyncRPCOperation::getResult() const {
 Value AsyncRPCOperation::getStatus() const {
     OperationStatus status = this->getState();
     Object obj;
-    obj.push_back(Pair("id", this->getId()));
+    obj.push_back(Pair("id", this->id_));
     obj.push_back(Pair("status", OperationStatusMap[status]));
     obj.push_back(Pair("creation_time", this->creation_time_));
     // TODO: Issue #1354: There may be other useful metadata to return to the user.
diff --git a/src/asyncrpcoperation.h b/src/asyncrpcoperation.h
index 9da76791..bcfda892 100644
--- a/src/asyncrpcoperation.h
+++ b/src/asyncrpcoperation.h
@@ -10,6 +10,10 @@
 #include <atomic>
 #include <map>
 #include <chrono>
+#include <memory>
+#include <thread>
+#include <utility>
+#include <future>
 
 #include "json/json_spirit_value.h"
 #include "json/json_spirit_utils.h"
@@ -71,10 +75,12 @@ public:
     std::string getStateAsString() const;
     
     int getErrorCode() const {
+        std::lock_guard<std::mutex> guard(lock_);
         return error_code_;
     }
 
     std::string getErrorMessage() const {
+        std::lock_guard<std::mutex> guard(lock_);
         return error_message_;
     }
 
@@ -106,6 +112,7 @@ protected:
     // allow subclasses of AsyncRPCOperation the ability to access and update
     // internal state.  Currently, all operations are executed in a single-thread
     // by a single worker.
+    mutable std::mutex lock_;   // lock on this when read/writing non-atomics
     Value result_;
     int error_code_;
     std::string error_message_;
@@ -120,14 +127,17 @@ protected:
     }
 
     void set_error_code(int errorCode) {
+        std::lock_guard<std::mutex> guard(lock_);
         this->error_code_ = errorCode;
     }
 
     void set_error_message(std::string errorMessage) {
+        std::lock_guard<std::mutex> guard(lock_);
         this->error_message_ = errorMessage;
     }
     
     void set_result(Value v) {
+        std::lock_guard<std::mutex> guard(lock_);
         this->result_ = v;
     }
     
@@ -136,15 +146,8 @@ private:
     // Derived classes should write their own copy constructor and assignment operators
     AsyncRPCOperation(const AsyncRPCOperation& orig);
     AsyncRPCOperation& operator=( const AsyncRPCOperation& other );
-    
-    void set_id(AsyncRPCOperationId id) {
-        this->id_ = id;
-    }
-    
-    void set_creation_time(int64_t creationTime) {
-        this->creation_time_ = creationTime;
-    }
 
+    // Initialized in the operation constructor, never to be modified again.
     AsyncRPCOperationId id_;
     int64_t creation_time_;
 };