From a1919bc4ef26b7289e29aab429ad77ccae08db70 Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Thu, 16 Apr 2020 06:45:34 -0700 Subject: [PATCH] Flipper | Stop exceptions being thrown for indicating that no Flipper desktop client is available Summary: Originally, Flipper would use exceptions for the control flow of determining if there's no desktop client available. It would catch an exception and then kick off another attempt. This is cool, but if you are debugging an application where Flipper is enabled, and you set a breakpoint to detect all thrown exceptions, then you'll see an exception thrown inside Flipper logic every couple of seconds. That makes it hard to debug what you're trying to do. There's a workaround where you can simply open Flipper, but that's a bit annoying. This diff changes the logic in Flipper to not use exceptions for the part of connection where it's doing the initial connect. We could apply this to `doCertificateExchange` as well, but that's actually an error there so I don't see the need to do it there as well. Reviewed By: jknoxville Differential Revision: D21016737 fbshipit-source-id: cbdfe2794b4d38a9e3b8304ebee845655bb26ae5 --- .../Flipper/FlipperConnectionManagerImpl.cpp | 60 ++++++++++++------- xplat/Flipper/FlipperConnectionManagerImpl.h | 2 +- 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/xplat/Flipper/FlipperConnectionManagerImpl.cpp b/xplat/Flipper/FlipperConnectionManagerImpl.cpp index 9cbcfb5e9..de5748aa7 100644 --- a/xplat/Flipper/FlipperConnectionManagerImpl.cpp +++ b/xplat/Flipper/FlipperConnectionManagerImpl.cpp @@ -129,32 +129,30 @@ void FlipperConnectionManagerImpl::startSync() { if (isClientSetupStep) { doCertificateExchange(); } else { - connectSecurely(); + if (!connectSecurely()) { + // The expected code path when flipper desktop is not running. + // Don't count as a failed attempt, or it would invalidate the + // connection files for no reason. On iOS devices, we can always connect + // to the local port forwarding server even when it can't connect to + // flipper. In that case we get a Network error instead of a Port not + // open error, so we treat them the same. + step->fail( + "No route to flipper found. Is flipper desktop running? Retrying..."); + reconnect(); + } } step->complete(); } catch (const folly::AsyncSocketException& e) { - if (e.getType() == folly::AsyncSocketException::NOT_OPEN || - e.getType() == folly::AsyncSocketException::NETWORK_ERROR) { - // The expected code path when flipper desktop is not running. - // Don't count as a failed attempt, or it would invalidate the connection - // files for no reason. On iOS devices, we can always connect to the local - // port forwarding server even when it can't connect to flipper. In that - // case we get a Network error instead of a Port not open error, so we - // treat them the same. - step->fail( - "No route to flipper found. Is flipper desktop running? Retrying..."); + if (e.getType() == folly::AsyncSocketException::SSL_ERROR) { + auto message = std::string(e.what()) + + "\nMake sure the date and time of your device is up to date."; + log(message); + step->fail(message); } else { - if (e.getType() == folly::AsyncSocketException::SSL_ERROR) { - auto message = std::string(e.what()) + - "\nMake sure the date and time of your device is up to date."; - log(message); - step->fail(message); - } else { - log(e.what()); - step->fail(e.what()); - } - failedConnectionAttempts_++; + log(e.what()); + step->fail(e.what()); } + failedConnectionAttempts_++; reconnect(); } catch (const std::exception& e) { log(e.what()); @@ -194,7 +192,7 @@ void FlipperConnectionManagerImpl::doCertificateExchange() { requestSignedCertFromFlipper(); } -void FlipperConnectionManagerImpl::connectSecurely() { +bool FlipperConnectionManagerImpl::connectSecurely() { rsocket::SetupParameters parameters; folly::SocketAddress address; @@ -216,7 +214,8 @@ void FlipperConnectionManagerImpl::connectSecurely() { contextStore_->getSSLContext(); auto connectingSecurely = flipperState_->start("Connect securely"); connectionIsTrusted_ = true; - client_ = + + auto newClient = rsocket::RSocket::createConnectedClient( std::make_unique( *connectionEventBase_->getEventBase(), @@ -227,9 +226,24 @@ void FlipperConnectionManagerImpl::connectSecurely() { std::chrono::seconds(connectionKeepaliveSeconds), // keepaliveInterval nullptr, // stats std::make_shared(this)) + .thenError([](const auto& e) { + if (e.getType() == folly::AsyncSocketException::NOT_OPEN || + e.getType() == folly::AsyncSocketException::NETWORK_ERROR) { + // This is the state where no Flipper desktop client is connected. + // We don't want an exception thrown here. + return std::unique_ptr(nullptr); + } + throw e; + }) .get(); + if (newClient.get() == nullptr) { + return false; + } + + client_ = std::move(newClient); connectingSecurely->complete(); failedConnectionAttempts_ = 0; + return true; } void FlipperConnectionManagerImpl::reconnect() { diff --git a/xplat/Flipper/FlipperConnectionManagerImpl.h b/xplat/Flipper/FlipperConnectionManagerImpl.h index 9763748ce..890430ac7 100644 --- a/xplat/Flipper/FlipperConnectionManagerImpl.h +++ b/xplat/Flipper/FlipperConnectionManagerImpl.h @@ -69,7 +69,7 @@ class FlipperConnectionManagerImpl : public FlipperConnectionManager { void startSync(); void doCertificateExchange(); - void connectSecurely(); + bool connectSecurely(); bool isCertificateExchangeNeeded(); void requestSignedCertFromFlipper(); bool isRunningInOwnThread();