mkcurl: allow reusing easy handle (#35)

* mkcurl: allow reusing easy handle

Closes #34

This should allow changes in mkcollector such that we can reuse an
existing cURL handle when submitting a batch of reports.

In turn, this means we can cut the time require for uploading reports
because wen can reuse the existing connection.

I'd like this to be included into the next OONI mobile release and
to do this there is a number of steps of which this is the first.

Related to ooni/probe#861

* Update mkbuild and regen

CMakeLists.txt

@@ -210,10 +210,10 @@ macro(MKSetRestrictiveCompilerFlags)
     if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
       set(MK_COMMON_FLAGS "${MK_COMMON_FLAGS} -Wtrampolines")
     endif()
-    set(MK_COMMON_FLAGS "${MK_COMMON_FLAGS} -Woverloaded-virtual")
-    set(MK_COMMON_FLAGS "${MK_COMMON_FLAGS} -Wreorder")
-    set(MK_COMMON_FLAGS "${MK_COMMON_FLAGS} -Wsign-promo")
-    set(MK_COMMON_FLAGS "${MK_COMMON_FLAGS} -Wnon-virtual-dtor")
+    set(MK_CXX_FLAGS "${MK_CXX_FLAGS} -Woverloaded-virtual")
+    set(MK_CXX_FLAGS "${MK_CXX_FLAGS} -Wreorder")
+    set(MK_CXX_FLAGS "${MK_CXX_FLAGS} -Wsign-promo")
+    set(MK_CXX_FLAGS "${MK_CXX_FLAGS} -Wnon-virtual-dtor")
     set(MK_COMMON_FLAGS "${MK_COMMON_FLAGS} -fstack-protector-all")
     if(NOT "${APPLE}" AND NOT "${MINGW}")
       set(MK_LD_FLAGS "${MK_LD_FLAGS} -Wl,-z,noexecstack")
@@ -368,6 +368,14 @@ add_test(
   NAME redirect_test COMMAND mkcurl-client --follow-redirect http://google.com
 )
 
+#
+# test: reuseconnection
+#
+
+add_test(
+  NAME reuseconnection COMMAND mkcurl-client https://www.google.com https://www.google.com/robots.txt https://www.google.com/favicon.ico
+)
+
 #
 # test: using_timeout
 #

MKBuild.yaml

@@ -46,3 +46,7 @@ tests:
       https://httpbin.org/put
   connect_to:
     command: mkcurl-client --connect-to www.google.com https://www.youtube.com
+  reuseconnection:
+    command: mkcurl-client https://www.google.com
+      https://www.google.com/robots.txt
+      https://www.google.com/favicon.ico

docker.sh

@@ -1,7 +1,7 @@
 #!/bin/sh -e
 # Autogenerated by 'mkbuild'; DO NOT EDIT!
 
-USAGE="Usage: $0 asan|clang|coverage|tsan|ubsan|vanilla"
+USAGE="Usage: $0 asan|clang|coverage|ubsan|vanilla"
 
 if [ $# -eq 1 ]; then
   INTERNAL=0
@@ -67,14 +67,14 @@ elif [ "$BUILD_TYPE" = "vanilla" ]; then
   export CMAKE_BUILD_TYPE="Release"
 
 else
-  echo "$0: BUILD_TYPE not in: asan, clang, coverage, tsan, ubsan, vanilla" 1>&2
+  echo "$0: BUILD_TYPE not in: asan, clang, coverage, ubsan, vanilla" 1>&2
   exit 1
 fi
 
 # Configure, make, and make check equivalent
-mkdir build
-cd build
-cmake -GNinja -DCMAKE_BUILD_TYPE=$CMAKE_BUILD_TYPE ..
+mkdir -p build/$BUILD_TYPE
+cd build/$BUILD_TYPE
+cmake -GNinja -DCMAKE_BUILD_TYPE=$CMAKE_BUILD_TYPE ../../
 cmake --build . -- -v
 ctest --output-on-failure -a -j8
 

mkcurl-client.cpp

@@ -20,7 +20,7 @@
 static void usage() {
   // clang-format off
   std::clog << "\n";
-  std::clog << "Usage: mkcurl-client [options] <url>\n";
+  std::clog << "Usage: mkcurl-client [options] <url>...\n";
   std::clog << "\n";
   std::clog << "Options can start with either a single dash (i.e. -option) or\n";
   std::clog << "a double dash (i.e. --option). Available options:\n";
@@ -76,8 +76,8 @@ static void summary(mk::curl::Response &res) {
 
 int main(int, char **argv) {
   mk::curl::Request req;
+  argh::parser cmdline;
   {
-    argh::parser cmdline;
     cmdline.add_param("ca-bundle-path");
     cmdline.add_param("connect-to");
     cmdline.add_param("data");
@@ -129,20 +129,26 @@ int main(int, char **argv) {
       }
     }
     auto sz = cmdline.pos_args().size();
-    if (sz != 2) {
+    if (sz < 2) {
       // LCOV_EXCL_START
       usage();
       exit(EXIT_FAILURE);
       // LCOV_EXCL_STOP
     }
-    req.url = cmdline.pos_args()[1];
   }
-  mk::curl::Response res = mk::curl::perform(req);
-  summary(res);
-  if (res.error != 0 || res.status_code != 200) {
-    // LCOV_EXCL_START
-    std::clog << "FATAL: the request did not succeed" << std::endl;
-    exit(EXIT_FAILURE);
-    // LCOV_EXCL_STOP
+  auto exitcode = EXIT_SUCCESS;
+  mk::curl::Client client;
+  for (size_t sz = 1; sz < cmdline.pos_args().size(); ++sz) {
+    mk::curl::Request real_request{req};
+    real_request.url = cmdline.pos_args()[sz];
+    mk::curl::Response res = client.perform(real_request);
+    summary(res);
+    if (res.error != 0 || res.status_code != 200) {
+      // LCOV_EXCL_START
+      std::clog << "FATAL: the request did not succeed" << std::endl;
+      exitcode = EXIT_FAILURE;
+      // LCOV_EXCL_STOP
+    }
   }
+  exit(exitcode);
 }

mkcurl.hpp

@@ -6,6 +6,7 @@
 
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -108,6 +109,48 @@ struct Response {
   std::string http_version;
 };
 
+/// Client is an HTTP client. This class is movable but not copyable because
+/// at any give moment we want only a single client instance.
+///
+/// This is because a Client wraps a cURL handle and:
+///
+///   Handles. You must never share the same handle in multiple threads. You
+///   can pass the handles around among threads, but you must never use a single
+///   handle from more than one thread at any given time.
+///
+/// (see <https://curl.haxx.se/libcurl/c/threadsafe.html>) hence having single
+/// ownership in place guarantees that we cannot make such mistakes.
+class Client {
+ public:
+  /// Client creates a new client.
+  Client() noexcept;
+
+  /// Client is the deleted copy constructor.
+  Client(const Client &) noexcept = delete;
+
+  /// Client is the deleted copy assignment.
+  Client &operator=(const Client &) noexcept = delete;
+
+  /// Client is the move constructor.
+  Client(Client &&) noexcept;
+
+  /// Client is the move assignment.
+  Client &operator=(Client &&) noexcept;
+
+  /// ~Client is the destructor.
+  ~Client() noexcept;
+
+  /// perform performs @p request and returns the Response.
+  Response perform(const Request &request) noexcept;
+
+ private:
+  // Impl is the implementation of a client.
+  class Impl;
+
+  // impl_ is a unique pointer to the opaque implementation.
+  std::unique_ptr<Impl> impl_;
+};
+
 /// perform performs @p request and returns the Response.
 Response perform(const Request &request) noexcept;
 
@@ -123,7 +166,6 @@ Response perform(const Request &request) noexcept;
 
 #include <algorithm>
 #include <chrono>
-#include <memory>
 #include <sstream>
 
 #include <curl/curl.h>
@@ -166,6 +208,19 @@ struct mkcurl_deleter {
 // mkcurl_uptr is a unique pointer to a CURL handle.
 using mkcurl_uptr = std::unique_ptr<CURL, mkcurl_deleter>;
 
+// Client::Impl contains the implementation of a client.
+class Client::Impl {
+ public:
+  mkcurl_uptr handle;
+  Impl() noexcept = default;
+  Impl(const Impl &) noexcept = delete;
+  Impl &operator=(const Impl &) noexcept = delete;
+  Impl(Impl &&) noexcept = delete;
+  Impl &operator=(Impl &&) noexcept = delete;
+  ~Impl() noexcept;
+};
+Client::Impl::~Impl() noexcept = default; // Avoid `-Wweak-vtables`
+
 // mkcurl_slist is a curl_slist with RAII semantic.
 struct mkcurl_slist {
   // mkcurl_slist is the default constructor.
@@ -355,19 +410,37 @@ static CURLcode perform_and_retry(
   return rv;
 }
 
-Response perform(const Request &req) noexcept {
-  mkcurl_uptr handle;
-  {
+// perform2 will use @p handle to perform @p req. If @p handle is not set
+// we will initialise it. Otherwise the @p handle argument options are
+// reset to allow constructing a fresh HTTP request. Still, in such case, we'll
+// reuse existing connections etc. @return the response.
+static Response perform2(mkcurl_uptr &handle, const Request &req) noexcept {
+  Response res;
+  if (!handle) {
     CURL *handlep = curl_easy_init();
     MKCURL_HOOK_ALLOC(curl_easy_init, handlep, curl_easy_cleanup);
     handle.reset(handlep);
-  }
-  Response res;
-  if (!handle) {
-    res.error = CURLE_OUT_OF_MEMORY;
-    mkcurl_log(res.logs, "curl_easy_init() failed");
-    return res;
-  }
+    if (!handle) {
+      res.error = CURLE_OUT_OF_MEMORY;
+      mkcurl_log(res.logs, "curl_easy_init() failed");
+      return res;
+    }
+    // FALLTHROUGH
+  }
+  /*
+   * From <https://curl.haxx.se/libcurl/c/curl_easy_reset.html>:
+   *
+   *   Re-initializes all options previously set on a specified CURL handle to
+   *   the default values. This puts back the handle to the same state as it was
+   *   in when it was just created with curl_easy_init.
+   *
+   *   It does not change the following information kept in the handle: live
+   *   connections, the Session ID cache, the DNS cache, the cookies and shares.
+   *
+   * So, this allows us to reuse the existing connections with a completely
+   * new request whose options can be set from scratch below.
+   */
+  curl_easy_reset(handle.get());
   mkcurl_slist headers;  // This must have function scope
   for (auto &s : req.headers) {
     curl_slist *slistp = curl_slist_append(headers.p, s.c_str());
@@ -672,6 +745,18 @@ Response perform(const Request &req) noexcept {
   return res;
 }
 
+Client::Client() noexcept { impl_.reset(new Client::Impl); }
+Client::Client(Client &&) noexcept = default;
+Client &Client::operator=(Client &&) noexcept = default;
+Client::~Client() noexcept = default;
+Response Client::perform(const Request &req) noexcept {
+  return perform2(impl_->handle, req);
+}
+
+Response perform(const Request &req) noexcept {
+  return Client{}.perform(req);
+}
+
 }  // namespace curl
 }  // namespace mk
 #endif  // MKCURL_INLINE_IMPL