Allow invoking callbacks from HTTPConnector.reset() and enable them i…

…n ConnectionHelper destructor to track cancellations

Summary:
HTTPConnector intentionally avoids invoking the callbacks when reset() is called.
I found D1142299, which added the logic, presumably, to enable reusing the HTTPConnector for the next health check, without invoking the callbacks on cancellation of the outstanding connection attempts. I don't think this particular code path needs it anymore, but some of the other consumers came to rely on the fact that callbacks are not invoked on reset().
In the rev proxy, reset() [ is explicitly called from the ConnectionHelper](https://www.internalfb.com/code/fbsource/[95a94eb3ce59841ad2a5d9f88727b57d600e21ab]/fbcode/proxygen/facebook/revproxy/ConnectionHelper.cpp?lines=52-610) during its destruction to cancel any pending requests, not to reuse the connector. I need the callbacks to be invoked when this happens to account for the cancelation in the metric that I am adding. An easy way of doing it is to provide a flag, defaulting to the existing behavior, to enable the callbacks.

Splitting out this small change to narrow down the blast radius and test it separately.

A potential alternative solution would be to add a new onCanceled() callback and call that one unconditionally on cancellation. I can do this more invasive change if something doesn't work out with this one.

Reviewed By: afrind

Differential Revision: D61172051

fbshipit-source-id: 7d69e3efa5f0cbace54c3822cca8610d826a77a6

proxygen/lib/http/HTTPConnector.cpp

@@ -10,6 +10,7 @@
 
 #include <folly/io/SocketOptionMap.h>
 #include <folly/io/async/AsyncSSLSocket.h>
+#include <folly/logging/xlog.h>
 #include <proxygen/lib/http/codec/DefaultHTTPCodecFactory.h>
 #include <proxygen/lib/http/codec/HTTP1xCodec.h>
 #include <proxygen/lib/http/codec/HTTP2Codec.h>
@@ -32,16 +33,22 @@ HTTPConnector::HTTPConnector(Callback* callback,
     : cb_(CHECK_NOTNULL(callback)),
       timeout_(timeout),
       httpCodecFactory_(std::make_unique<DefaultHTTPCodecFactory>()) {
+  XLOG(DBG4) << "HTTPConnector";
 }
 
 HTTPConnector::~HTTPConnector() {
-  reset();
+  XLOG(DBG4) << "~HTTPConnector";
+  reset(false);
 }
 
-void HTTPConnector::reset() {
+void HTTPConnector::reset(bool invokeCallbacks) {
+  XLOG(DBG4) << "reset invokeCallbacks=" << invokeCallbacks;
   if (socket_) {
     auto cb = cb_;
-    cb_ = nullptr;
+    if (!invokeCallbacks) {
+      cb_ = nullptr;
+    }
+    XLOG(DBG4) << "socket_.reset()";
     socket_.reset(); // This invokes connectError() but will be ignored
     cb_ = cb;
   }
@@ -61,6 +68,7 @@ void HTTPConnector::connect(EventBase* eventBase,
                             const SocketOptionMap& socketOptions,
                             const folly::SocketAddress& bindAddr) {
 
+  XLOG(DBG4) << "connect";
   DCHECK(!isBusy());
   transportInfo_ = wangle::TransportInfo();
   transportInfo_.secure = false;
@@ -80,6 +88,7 @@ void HTTPConnector::connectSSL(EventBase* eventBase,
                                const folly::SocketAddress& bindAddr,
                                const std::string& serverName) {
 
+  XLOG(DBG4) << "connectSSL";
   DCHECK(!isBusy());
   transportInfo_ = wangle::TransportInfo();
   transportInfo_.secure = true;

proxygen/lib/http/HTTPConnector.h

@@ -70,11 +70,12 @@ class HTTPConnector : protected folly::AsyncSocket::ConnectCallback {
   ~HTTPConnector() override;
 
   /**
-   * Reset the object so that it can begin a new connection. No callbacks
-   * will be invoked as a result of executing this function. After this
-   * function returns, isBusy() will return false.
+   * Reset the object so that it can begin a new connection. Callbacks
+   * are suppressed by default, but can be enabled, when the intent is to cancel
+   * the outstanding work. After this function returns, isBusy() will return
+   * false.
    */
-  void reset();
+  void reset(bool invokeCallbacks = false);
 
   /**
    * Sets the plain text protocol to use after the connection