From d37c64c3298cbd6e943688a2759b192636c38d8b Mon Sep 17 00:00:00 2001
From: Matt Galloway <mattjgalloway@fb.com>
Date: Wed, 4 Dec 2019 07:31:41 -0800
Subject: [PATCH] Flipper | Fix potential for deadlock when stopping Flipper

Summary:
There was an interesting problem in Flipper which I encountered. On closing our app we would deadlock. This was happening when joining the Flipper thread.

Upon investigation I found that it happened only when Flipper was not connected to the app. And digging in I found that it's because we are continually trying to connect the `FlipperClient` to the Flipper app when there is no connection. This works great, but it continues even if you have asked Flipper to terminate. So this meant that the `EventBase` never gets chance to shutdown, as it keeps getting new work to do.

The fix is to add a flag to `FlipperConnectionManagerImpl` to say if the conection is started or not. Then, if it's not started and we get a request to retry connection, then we will fail it.

Reviewed By: jknoxville

Differential Revision: D18780622

fbshipit-source-id: cce82cbb5c54e6d92ea16644c8a247bd2578ae26
---
 .../Flipper/FlipperConnectionManagerImpl.cpp  | 20 +++++++++++++++++++
 xplat/Flipper/FlipperConnectionManagerImpl.h  |  1 +
 2 files changed, 21 insertions(+)

diff --git a/xplat/Flipper/FlipperConnectionManagerImpl.cpp b/xplat/Flipper/FlipperConnectionManagerImpl.cpp
index 7c74d8fca..8458a458b 100644
--- a/xplat/Flipper/FlipperConnectionManagerImpl.cpp
+++ b/xplat/Flipper/FlipperConnectionManagerImpl.cpp
@@ -91,6 +91,12 @@ FlipperConnectionManagerImpl::~FlipperConnectionManagerImpl() {
 }
 
 void FlipperConnectionManagerImpl::start() {
+  if (isStarted_) {
+    log("Already started");
+    return;
+  }
+  isStarted_ = true;
+
   auto step = flipperState_->start("Start connection thread");
 
   folly::makeFuture()
@@ -103,6 +109,10 @@ void FlipperConnectionManagerImpl::start() {
 }
 
 void FlipperConnectionManagerImpl::startSync() {
+  if (!isStarted_) {
+    log("Not started");
+    return;
+  }
   if (!isRunningInOwnThread()) {
     log(WRONG_THREAD_EXIT_MSG);
     return;
@@ -219,6 +229,10 @@ void FlipperConnectionManagerImpl::connectSecurely() {
 }
 
 void FlipperConnectionManagerImpl::reconnect() {
+  if (!isStarted_) {
+    log("Not started");
+    return;
+  }
   folly::makeFuture()
       .via(flipperEventBase_->getEventBase())
       .delayed(std::chrono::seconds(reconnectIntervalSeconds))
@@ -226,6 +240,12 @@ void FlipperConnectionManagerImpl::reconnect() {
 }
 
 void FlipperConnectionManagerImpl::stop() {
+  if (!isStarted_) {
+    log("Not started");
+    return;
+  }
+  isStarted_ = false;
+
   if (client_) {
     client_->disconnect();
   }
diff --git a/xplat/Flipper/FlipperConnectionManagerImpl.h b/xplat/Flipper/FlipperConnectionManagerImpl.h
index 3989e7684..9763748ce 100644
--- a/xplat/Flipper/FlipperConnectionManagerImpl.h
+++ b/xplat/Flipper/FlipperConnectionManagerImpl.h
@@ -53,6 +53,7 @@ class FlipperConnectionManagerImpl : public FlipperConnectionManager {
 
  private:
   bool isOpen_ = false;
+  bool isStarted_ = false;
   Callbacks* callbacks_;
   DeviceData deviceData_;
   std::shared_ptr<FlipperState> flipperState_;