diff --git a/xplat/Flipper/FlipperState.cpp b/xplat/Flipper/FlipperState.cpp index 8c3589d63..81426ce9c 100644 --- a/xplat/Flipper/FlipperState.cpp +++ b/xplat/Flipper/FlipperState.cpp @@ -6,9 +6,9 @@ */ #include "FlipperState.h" +#include #include "FlipperStateUpdateListener.h" #include "FlipperStep.h" -#include #if FLIPPER_DEBUG_LOG #include "Log.h" @@ -22,42 +22,64 @@ using namespace facebook::flipper; FlipperState::FlipperState() : logs("") {} void FlipperState::setUpdateListener( std::shared_ptr listener) { + std::lock_guard lock(mutex); mListener = listener; } void FlipperState::started(std::string step) { + std::shared_ptr localListener; + { + std::lock_guard 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 localListener; + { + std::lock_guard 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 localListener; + { + std::lock_guard 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 lock(mutex); return logs; } std::vector FlipperState::getStateElements() { + std::lock_guard lock(mutex); std::vector v; for (auto stepName : insertOrder) { v.push_back(StateElement(stepName, stateMap[stepName])); @@ -77,6 +101,8 @@ std::vector FlipperState::getStateElements() { } std::shared_ptr 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(step_name, this); } diff --git a/xplat/Flipper/FlipperState.h b/xplat/Flipper/FlipperState.h index 95bc1292a..64e853e66 100644 --- a/xplat/Flipper/FlipperState.h +++ b/xplat/Flipper/FlipperState.h @@ -7,10 +7,11 @@ #pragma once +#include #include +#include #include #include -#include 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); std::string getState(); std::vector 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 mListener = nullptr; std::string logs; std::vector insertOrder;