Compiles, doesn't work

Expensify · Jan 30, 2020 · 56d3b88 · 56d3b88
1 parent f27e048
commit 56d3b88
Show file tree

Hide file tree

Showing 12 changed files with 302 additions and 401 deletions.
diff --git a/BedrockCommand.cpp b/BedrockCommand.cpp
@@ -29,9 +29,7 @@ BedrockCommand::~BedrockCommand() {
     for (auto request : httpsRequests) {
         request->manager.closeTransaction(request);
     }
-    if (countCommand) {
-        _commandCount--;
-    }
+    _commandCount--;
     if (deallocator && peekData) {
         deallocator(peekData);
     }
@@ -50,39 +48,9 @@ BedrockCommand::BedrockCommand(SQLiteCommand&& from, int dontCount) :
     peekData(nullptr),
     deallocator(nullptr),
     _inProgressTiming(INVALID, 0, 0),
-    _timeout(_getTimeout(request)),
-    countCommand(dontCount != DONT_COUNT)
+    _timeout(_getTimeout(request))
 {
     _init();
-    if (countCommand) {
-        _commandCount++;
-    }
-}
-
-BedrockCommand::BedrockCommand(BedrockCommand&& from) :
-    SQLiteCommand(move(from)),
-    httpsRequests(move(from.httpsRequests)),
-    priority(from.priority),
-    peekCount(from.peekCount),
-    processCount(from.processCount),
-    peekedBy(from.peekedBy),
-    processedBy(from.processedBy),
-    repeek(from.repeek),
-    timingInfo(from.timingInfo),
-    onlyProcessOnSyncThread(from.onlyProcessOnSyncThread),
-    crashIdentifyingValues(*this, move(from.crashIdentifyingValues)),
-    peekData(from.peekData),
-    deallocator(from.deallocator),
-    _inProgressTiming(from._inProgressTiming),
-    _timeout(from._timeout),
-    countCommand(true)
-{
-    // The move constructor (and likewise, the move assignment operator), don't simply copy these pointer values, but
-    // they clear them from the old object, so that when its destructor is called, the HTTPS transactions aren't
-    // closed.
-    from.httpsRequests.clear();
-    from.peekData = nullptr;
-    from.deallocator = nullptr;
     _commandCount++;
 }
 
@@ -99,8 +67,7 @@ BedrockCommand::BedrockCommand(SData&& _request) :
     peekData(nullptr),
     deallocator(nullptr),
     _inProgressTiming(INVALID, 0, 0),
-    _timeout(_getTimeout(request)),
-    countCommand(true)
+    _timeout(_getTimeout(request))
 {
     _init();
     _commandCount++;
@@ -119,68 +86,12 @@ BedrockCommand::BedrockCommand(SData _request) :
     peekData(nullptr),
     deallocator(nullptr),
     _inProgressTiming(INVALID, 0, 0),
-    _timeout(_getTimeout(request)),
-    countCommand(true)
+    _timeout(_getTimeout(request))
 {
     _init();
     _commandCount++;
 }
 
-BedrockCommand& BedrockCommand::operator=(BedrockCommand&& from) {
-    if (this != &from) {
-        // The current incarnation of this object is going away, if it had an httpsRequest, we'll need to destroy it,
-        // or it will leak and never get cleaned up.
-        for (auto request : httpsRequests) {
-            if (!request->response) {
-                SWARN("Closing unfinished httpRequest by assigning over it. This was probably a mistake.");
-            }
-            request->manager.closeTransaction(request);
-        }
-        httpsRequests = move(from.httpsRequests);
-        from.httpsRequests.clear();
-
-        // Same here, deallocate current data.
-        if (deallocator && peekData) {
-            deallocator(peekData);
-        }
-
-        // Update our other properties.
-        peekCount = from.peekCount;
-        processCount = from.processCount;
-        peekedBy = from.peekedBy;
-        processedBy = from.processedBy;
-        repeek = from.repeek;
-        priority = from.priority;
-        timingInfo = from.timingInfo;
-        onlyProcessOnSyncThread = from.onlyProcessOnSyncThread;
-        crashIdentifyingValues = move(from.crashIdentifyingValues);
-        peekData = move(from.peekData);
-        deallocator = move(from.deallocator);
-        _inProgressTiming = from._inProgressTiming;
-        _timeout = from._timeout;
-
-        // If countCommand is changing, update the count.
-        if (countCommand && !from.countCommand) {
-            // this command is no longer counted.
-            _commandCount--;
-        } else if (!countCommand && from.countCommand) {
-            // We need to start counting this command.
-            _commandCount++;
-        }
-        // Now set to match the existing command.
-        countCommand = from.countCommand;
-
-        // Don't delete when the old object is destroyed.
-        from.peekData = nullptr;
-        from.deallocator = nullptr;
-
-        // And call the base class's move constructor as well.
-        SQLiteCommand::operator=(move(from));
-    }
-
-    return *this;
-}
-
 void BedrockCommand::_init() {
     // Initialize the priority, if supplied.
     if (request.isSet("priority")) {
@@ -338,24 +249,24 @@ void BedrockCommand::finalizeTimingInfo() {
 
 // pop and push specializations for SSynchronizedQueue that record timing info.
 template<>
-BedrockCommand SSynchronizedQueue<BedrockCommand>::pop() {
+unique_ptr<BedrockCommand> SSynchronizedQueue<unique_ptr<BedrockCommand>>::pop() {
     SAUTOLOCK(_queueMutex);
     if (!_queue.empty()) {
-        BedrockCommand item = move(_queue.front());
+        unique_ptr<BedrockCommand> item = move(_queue.front());
         _queue.pop_front();
-        item.stopTiming(BedrockCommand::QUEUE_SYNC);
+        item->stopTiming(BedrockCommand::QUEUE_SYNC);
         return item;
     }
     throw out_of_range("No commands");
 }
 
 template<>
-void SSynchronizedQueue<BedrockCommand>::push(BedrockCommand&& cmd) {
+void SSynchronizedQueue<unique_ptr<BedrockCommand>>::push(unique_ptr<BedrockCommand>&& cmd) {
     SAUTOLOCK(_queueMutex);
-    SINFO("Enqueuing command '" << cmd.request.methodLine << "', with " << _queue.size() << " commands already queued.");
+    SINFO("Enqueuing command '" << cmd->request.methodLine << "', with " << _queue.size() << " commands already queued.");
     // Just add to the queue
     _queue.push_back(move(cmd));
-    _queue.back().startTiming(BedrockCommand::QUEUE_SYNC);
+    _queue.back()->startTiming(BedrockCommand::QUEUE_SYNC);
 
     // Write arbitrary buffer to the pipe so any subscribers will be awoken.
     // **NOTE: 1 byte so write is atomic.

diff --git a/BedrockCommand.h b/BedrockCommand.h
@@ -23,9 +23,6 @@ class BedrockCommand : public SQLiteCommand {
         QUEUE_SYNC,
     };
 
-    // used to create commands that don't count towards the total number of commands.
-    static constexpr int DONT_COUNT = 1;
-
     // Times in *milliseconds*.
     static const uint64_t DEFAULT_TIMEOUT = 290'000; // 290 seconds, so clients can have a 5 minute timeout.
     static const uint64_t DEFAULT_TIMEOUT_FORGET = 60'000 * 60; // 1 hour for `connection: forget` commands.
@@ -34,9 +31,6 @@ class BedrockCommand : public SQLiteCommand {
     // Constructor to convert from an existing SQLiteCommand (by move).
     BedrockCommand(SQLiteCommand&& from, int dontCount = 0);
 
-    // Move constructor.
-    BedrockCommand(BedrockCommand&& from);
-
     // Constructor to initialize via a request object (by move).
     BedrockCommand(SData&& _request);
 
@@ -46,9 +40,6 @@ class BedrockCommand : public SQLiteCommand {
     // Destructor.
     ~BedrockCommand();
 
-    // Move assignment operator.
-    BedrockCommand& operator=(BedrockCommand&& from);
-
     // Start recording time for a given action type.
     void startTiming(TIMING_INFO type);
 
@@ -147,6 +138,4 @@ class BedrockCommand : public SQLiteCommand {
     uint64_t _timeout;
 
     static atomic<size_t> _commandCount;
-
-    bool countCommand;
 };
diff --git a/BedrockCommandQueue.cpp b/BedrockCommandQueue.cpp
@@ -1,23 +1,23 @@
 #include <BedrockCommandQueue.h>
 
-void BedrockCommandQueue::startTiming(BedrockCommand& command) {
-    command.startTiming(BedrockCommand::QUEUE_WORKER);
+void BedrockCommandQueue::startTiming(unique_ptr<BedrockCommand>& command) {
+    command->startTiming(BedrockCommand::QUEUE_WORKER);
 }
 
-void BedrockCommandQueue::stopTiming(BedrockCommand& command) {
-    command.stopTiming(BedrockCommand::QUEUE_WORKER);
+void BedrockCommandQueue::stopTiming(unique_ptr<BedrockCommand>& command) {
+    command->stopTiming(BedrockCommand::QUEUE_WORKER);
 }
 
 BedrockCommandQueue::BedrockCommandQueue() :
-  SScheduledPriorityQueue<BedrockCommand>(function<void(BedrockCommand&)>(startTiming), function<void(BedrockCommand&)>(stopTiming))
+  SScheduledPriorityQueue<unique_ptr<BedrockCommand>>(function<void(unique_ptr<BedrockCommand>&)>(startTiming), function<void(unique_ptr<BedrockCommand>&)>(stopTiming))
 { }
 
 list<string> BedrockCommandQueue::getRequestMethodLines() {
     list<string> returnVal;
     SAUTOLOCK(_queueMutex);
     for (auto& queue : _queue) {
         for (auto& entry : queue.second) {
-            returnVal.push_back(entry.second.item.request.methodLine);
+            returnVal.push_back(entry.second.item->request.methodLine);
         }
     }
     return returnVal;
@@ -64,9 +64,9 @@ void BedrockCommandQueue::abandonFutureCommands(int msInFuture) {
     }
 }
 
-void BedrockCommandQueue::push(BedrockCommand&& command) {
-    BedrockCommand::Priority priority = command.priority;
-    uint64_t executionTime = command.request.calcU64("commandExecuteTime");
-    uint64_t timeout = command.timeout();
-    SScheduledPriorityQueue<BedrockCommand>::push(move(command), priority, executionTime, timeout);
+void BedrockCommandQueue::push(unique_ptr<BedrockCommand>&& command) {
+    BedrockCommand::Priority priority = command->priority;
+    uint64_t executionTime = command->request.calcU64("commandExecuteTime");
+    uint64_t timeout = command->timeout();
+    SScheduledPriorityQueue<unique_ptr<BedrockCommand>>::push(move(command), priority, executionTime, timeout);
 }
diff --git a/BedrockCommandQueue.h b/BedrockCommandQueue.h
@@ -3,13 +3,13 @@
 #include <libstuff/SScheduledPriorityQueue.h>
 #include "BedrockCommand.h"
 
-class BedrockCommandQueue : public SScheduledPriorityQueue<BedrockCommand> {
+class BedrockCommandQueue : public SScheduledPriorityQueue<unique_ptr<BedrockCommand>> {
   public:
     BedrockCommandQueue();
 
     // Functions to start and stop timing on the commands when they're inserted/removed from the queue.
-    static void startTiming(BedrockCommand& command);
-    static void stopTiming(BedrockCommand& command);
+    static void startTiming(unique_ptr<BedrockCommand>& command);
+    static void stopTiming(unique_ptr<BedrockCommand>& command);
 
     // Returns a list of all the method lines for all the requests currently queued. This function exists for state
     // reporting, and is called by BedrockServer when we receive a `Status` command.
@@ -19,5 +19,5 @@ class BedrockCommandQueue : public SScheduledPriorityQueue<BedrockCommand> {
     void abandonFutureCommands(int msInFuture);
 
     // Add an item to the queue. The queue takes ownership of the item and the caller's copy is invalidated.
-    void push(BedrockCommand&& command);
+    void push(unique_ptr<BedrockCommand>&& command);
 };