Add locking to FlipperState

Summary: FlipperState may be updated from multiple threads, so it needs synchronization. Note the comments in the diff about why we drop the lock before calling out to the update listener -- this can be changed if necessary in the future.

Reviewed By: jknoxville

Differential Revision: D18095471

fbshipit-source-id: 95d558394ae1a9b7583e5a61969e1eeda6448cff

xplat/Flipper/FlipperState.cpp

@@ -6,9 +6,9 @@
  */
 
 #include "FlipperState.h"
+#include <vector>
 #include "FlipperStateUpdateListener.h"
 #include "FlipperStep.h"
-#include <vector>
 
 #if FLIPPER_DEBUG_LOG
 #include "Log.h"
@@ -22,42 +22,64 @@ using namespace facebook::flipper;
 FlipperState::FlipperState() : logs("") {}
 void FlipperState::setUpdateListener(
     std::shared_ptr<FlipperStateUpdateListener> listener) {
+  std::lock_guard<std::mutex> lock(mutex);
   mListener = listener;
 }
 
 void FlipperState::started(std::string step) {
+  std::shared_ptr<FlipperStateUpdateListener> localListener;
+  {
+    std::lock_guard<std::mutex> lock(mutex);
 #if FLIPPER_DEBUG_LOG
-  log("[started] " + step);
+    log("[started] " + step);
 #endif
-  if (stateMap.find(step) == stateMap.end()) {
-    insertOrder.push_back(step);
+    if (stateMap.find(step) == stateMap.end()) {
+      insertOrder.push_back(step);
+    }
+    stateMap[step] = State::in_progress;
+    localListener = mListener;
   }
-  stateMap[step] = State::in_progress;
-  if (mListener) {
-    mListener->onUpdate();
+  // Need to drop the lock before issuing callback because the caller
+  // might call us back (and is responsible for their own locking).
+  if (localListener) {
+    localListener->onUpdate();
   }
 }
 
 void FlipperState::success(std::string step) {
+  std::shared_ptr<FlipperStateUpdateListener> localListener;
+  {
+    std::lock_guard<std::mutex> lock(mutex);
 #if FLIPPER_DEBUG_LOG
-  log("[finished] " + step);
+    log("[finished] " + step);
 #endif
-  logs = logs + "[Success] " + step + "\n";
-  stateMap[step] = State::success;
-  if (mListener) {
-    mListener->onUpdate();
+    logs = logs + "[Success] " + step + "\n";
+    stateMap[step] = State::success;
+    localListener = mListener;
+  }
+  // Need to drop the lock before issuing callback because the caller
+  // might call us back (and is responsible for their own locking).
+  if (localListener) {
+    localListener->onUpdate();
   }
 }
 
 void FlipperState::failed(std::string step, std::string errorMessage) {
-  std::string message = "[Failed] " + step + ": " + errorMessage;
+  std::shared_ptr<FlipperStateUpdateListener> localListener;
+  {
+    std::lock_guard<std::mutex> lock(mutex);
+    std::string message = "[Failed] " + step + ": " + errorMessage;
 #if FLIPPER_DEBUG_LOG
-  log(message);
+    log(message);
 #endif
-  logs = logs + message + "\n";
-  stateMap[step] = State::failed;
-  if (mListener) {
-    mListener->onUpdate();
+    logs = logs + message + "\n";
+    stateMap[step] = State::failed;
+    localListener = mListener;
+  }
+  // Need to drop the lock before issuing callback because the caller
+  // might call us back (and is responsible for their own locking).
+  if (localListener) {
+    localListener->onUpdate();
   }
 }
 
@@ -65,10 +87,12 @@ void FlipperState::failed(std::string step, std::string errorMessage) {
 // representation of the current state so the UI can show it in a more intuitive
 // way
 std::string FlipperState::getState() {
+  std::lock_guard<std::mutex> lock(mutex);
   return logs;
 }
 
 std::vector<StateElement> FlipperState::getStateElements() {
+  std::lock_guard<std::mutex> lock(mutex);
   std::vector<StateElement> v;
   for (auto stepName : insertOrder) {
     v.push_back(StateElement(stepName, stateMap[stepName]));
@@ -77,6 +101,8 @@ std::vector<StateElement> FlipperState::getStateElements() {
 }
 
 std::shared_ptr<FlipperStep> FlipperState::start(std::string step_name) {
+  // started() acquires the lock and we don't access any of our members below,
+  // so we needn't take the lock.
   started(step_name);
   return std::make_shared<FlipperStep>(step_name, this);
 }

xplat/Flipper/FlipperState.h

@@ -7,10 +7,11 @@
 
 #pragma once
 
+#include <map>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <vector>
-#include <map>
 
 class FlipperStep;
 class FlipperStateUpdateListener;
@@ -21,20 +22,23 @@ namespace flipper {
 enum State { success, in_progress, failed };
 
 class StateElement {
-public:
-  StateElement(std::string name, State state): name_(name), state_(state) {};
+ public:
+  StateElement(std::string name, State state) : name_(name), state_(state){};
   std::string name_;
   State state_;
 };
 
-}
-}
+} // namespace flipper
+} // namespace facebook
 
 class FlipperState {
   friend FlipperStep;
 
  public:
   FlipperState();
+  // Update listeners are responsible for their own
+  // synchronization. There is no guarantee about which thread they
+  // may be called on.
   void setUpdateListener(std::shared_ptr<FlipperStateUpdateListener>);
   std::string getState();
   std::vector<facebook::flipper::StateElement> getStateElements();
@@ -50,6 +54,7 @@ class FlipperState {
   void failed(std::string, std::string);
   void started(std::string);
 
+  std::mutex mutex; // Protects all our member variables.
   std::shared_ptr<FlipperStateUpdateListener> mListener = nullptr;
   std::string logs;
   std::vector<std::string> insertOrder;