Skip to content

Commit

Permalink
Fix race condition in Inspector shutdown
Browse files Browse the repository at this point in the history
Summary:
The `state_` member variable was getting destroyed before the `executor_` (because `state_` is declared after `executor_` https://stackoverflow.com/questions/2254263/order-of-member-constructor-and-destructor-calls). This lead to a race condition where items still pending on the `executor_` thread could try to run and access `state_` after it had already been reset.

Changelog:
[General][Fixed] - Fix race condition in Debug Inspector shutdown

Reviewed By: mhorowitz

Differential Revision: D24705062

fbshipit-source-id: e575d66322ab980b1a8c3e6083a0b3d40b9c944b
  • Loading branch information
MartinSherburn authored and facebook-github-bot committed Nov 9, 2020
1 parent 8e956b3 commit d021000
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 6 deletions.
1 change: 0 additions & 1 deletion ReactCommon/hermes/inspector/Inspector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ Inspector::Inspector(
}

Inspector::~Inspector() {
// TODO: think about expected detach flow
debugger_.setEventObserver(nullptr);
}

Expand Down
11 changes: 6 additions & 5 deletions ReactCommon/hermes/inspector/Inspector.h
Original file line number Diff line number Diff line change
Expand Up @@ -319,11 +319,6 @@ class Inspector : public facebook::hermes::debugger::EventObserver,
facebook::hermes::debugger::Debugger &debugger_;
InspectorObserver &observer_;

// All client methods (e.g. enable, setBreakpoint, resume, etc.) are executed
// on executor_ to prevent deadlocking on mutex_. See the implementation for
// more comments on the threading invariants used in this class.
std::unique_ptr<folly::Executor> executor_;

// All of the following member variables are guarded by mutex_.
std::mutex mutex_;
std::unique_ptr<InspectorState> state_;
Expand Down Expand Up @@ -360,6 +355,12 @@ class Inspector : public facebook::hermes::debugger::EventObserver,
// Are we currently waiting for a debugger to attach, because we
// requested 'pauseOnFirstStatement'?
bool awaitingDebuggerOnStart_;

// All client methods (e.g. enable, setBreakpoint, resume, etc.) are executed
// on executor_ to prevent deadlocking on mutex_. See the implementation for
// more comments on the threading invariants used in this class.
// NOTE: This needs to be declared LAST because it should be destroyed FIRST.
std::unique_ptr<folly::Executor> executor_;
};

} // namespace inspector
Expand Down

0 comments on commit d021000

Please sign in to comment.