Merge pull request #760 from Expensify/tyler-general-command-reset

Make command resetting a property of the base BedrockCommand class

BedrockCommand.cpp

@@ -112,6 +112,13 @@ bool BedrockCommand::areHttpsRequestsComplete() const {
     return true;
 }
 
+void BedrockCommand::reset(BedrockCommand::STAGE stage) {
+    if (stage == STAGE::PEEK) {
+        jsonContent.clear();
+        response.clear();
+    }
+}
+
 void BedrockCommand::finalizeTimingInfo() {
     uint64_t peekTotal = 0;
     uint64_t processTotal = 0;

BedrockCommand.h

@@ -23,6 +23,11 @@ class BedrockCommand : public SQLiteCommand {
         QUEUE_SYNC,
     };
 
+    enum class STAGE {
+        PEEK,
+        PROCESS
+    };
+
     // 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.
@@ -44,6 +49,14 @@ class BedrockCommand : public SQLiteCommand {
     // with any changes to the DB made by this plugin.
     virtual void process(SQLite& db) { STHROW("500 Base class process called"); }
 
+    // Reset the command after a commit conflict. This is called both before `peek` and `process`. Typically, we don't
+    // want to reset anything in `process`, because we may have specifically stored values there in `peek` that we want
+    // to access later. However, we provide this functionality to allow commands that make HTTPS requests to handle
+    // this extra case, as we run `peek` and `process` as separate transactions for these commands.
+    // The base class version of this does *not* change anything with regards to HTTPS requests. These are preserved
+    // across `reset` calls.
+    virtual void reset(STAGE stage);
+
     // Return the name of the plugin for this command.
     const string& getName() const;
 

BedrockCore.cpp

@@ -90,6 +90,7 @@ BedrockCore::RESULT BedrockCore::peekCommand(unique_ptr<BedrockCommand>& command
             _db.read("PRAGMA query_only = true;");
 
             // Peek.
+            command->reset(BedrockCommand::STAGE::PEEK);
             bool completed = command->peek(_db);
             SDEBUG("Plugin '" << command->getName() << "' peeked command '" << request.methodLine << "'");
 
@@ -199,6 +200,7 @@ BedrockCore::RESULT BedrockCore::processCommand(unique_ptr<BedrockCommand>& comm
             bool enable = command->shouldEnableQueryRewriting(_db, &handler);
             AutoScopeRewrite rewrite(enable, _db, handler);
             try {
+                command->reset(BedrockCommand::STAGE::PROCESS);
                 command->process(_db);
                 SDEBUG("Plugin '" << command->getName() << "' processed command '" << request.methodLine << "'");
             } catch (const SQLite::timeout_error& e) {

plugins/Jobs.cpp

@@ -117,12 +117,6 @@ bool BedrockJobsCommand::peek(SQLite& db) {
     // We can potentially change this, so we set it here.
     mockRequest = request.isSet("mockRequest");
 
-    // Reset the content object. It could have been written by a previous call to this function that conflicted in
-    // multi-write.
-    jsonContent.clear();
-    response.clear();
-
-    // ----------------------------------------------------------------------
     if (SIEquals(requestVerb, "GetJob") || SIEquals(requestVerb, "GetJobs")) {
         // - GetJob( name )
         // - GetJobs( name, numResults )
@@ -388,12 +382,6 @@ void BedrockJobsCommand::process(SQLite& db) {
     // Pull out some helpful variables
     const string& requestVerb = request.getVerb();
 
-    // Reset the content object. It could have been written by a previous call to this function that conflicted in
-    // multi-write.
-    jsonContent.clear();
-    response.clear();
-
-    // ----------------------------------------------------------------------
     if (SIEquals(requestVerb, "CreateJob") || SIEquals(requestVerb, "CreateJobs")) {
         // - CreateJob( name, [data], [firstRun], [repeat], [jobPriority], [unique], [parentJobID], [retryAfter] )
         //

test/clustertest/testplugin/TestPlugin.cpp

@@ -56,6 +56,14 @@ TestPluginCommand::TestPluginCommand(SQLiteCommand&& baseCommand, BedrockPlugin_
 {
 }
 
+void TestPluginCommand::reset(BedrockCommand::STAGE stage) {
+    if (stage == STAGE::PEEK) {
+        // We don't reset `pendingResult`, `urls`, or `chainedHTTPResponseContent` because they preserve state across
+        // multiple `repeek` calls.
+    }
+    BedrockCommand::reset(stage);
+};
+
 bool BedrockPlugin_TestPlugin::preventAttach() {
     return shouldPreventAttach;
 }
@@ -185,7 +193,7 @@ bool TestPluginCommand::peek(SQLite& db) {
                 STHROW("Pending Result flag set but no requests!");
             }
             // There was a previous request, let's record it's result.
-            response.content += httpsRequests.back()->fullRequest["Host"] + ":" + to_string(httpsRequests.back()->response) + "\n";
+            chainedHTTPResponseContent += httpsRequests.back()->fullRequest["Host"] + ":" + to_string(httpsRequests.back()->response) + "\n";
         }
         list<string> remainingURLs = SParseList(urls);
         if (remainingURLs.size()) {
@@ -320,7 +328,7 @@ void TestPluginCommand::process(SQLite& db) {
         return;
     } else if (SStartsWith(request.methodLine, "chainedrequest")) {
         // Note that we eventually got to process, though we write nothing to the DB.
-        response.content += "PROCESSED\n";
+        response.content = chainedHTTPResponseContent + "PROCESSED\n";
     }
 }
 

test/clustertest/testplugin/TestPlugin.h

@@ -39,10 +39,12 @@ class TestPluginCommand : public BedrockCommand {
     TestPluginCommand(SQLiteCommand&& baseCommand, BedrockPlugin_TestPlugin* plugin);
     virtual bool peek(SQLite& db);
     virtual void process(SQLite& db);
+    virtual void reset(BedrockCommand::STAGE stage) override;
 
   private:
     BedrockPlugin_TestPlugin& plugin() { return static_cast<BedrockPlugin_TestPlugin&>(*_plugin); }
 
     bool pendingResult;
+    string chainedHTTPResponseContent;
     string urls;
 };