Fix virtual call in ClientImpl::~ClientImpl() (#942)

* Fix virtual call in ClientImpl::~ClientImpl()

This fixes a warning in clang tidy:

> Call to virtual method 'ClientImpl::shutdown_ssl' during
> destruction bypasses virtual dispatch

ClientImpl::~ClientImpl() calls lock_socket_and_shutdown_and_close()
that itself calls shutdown_ssl().  However, shutdown_ssl() is virtual
and C++ does not perform virtual dispatch in destructors, which results
in the wrong overload being called.

This change adds a non-virtual shutdown_ssl_impl() function that is
called from ~SSLClient().  We also inline sock_socket_and_shutdown_and_close()
and removes the virtual call in ~ClientImpl().

* Inline and remove lock_socket_and_shutdown_and_close()

The function only has one caller.

example/ssesvr.cc

@@ -39,8 +39,8 @@ public:
 private:
   mutex m_;
   condition_variable cv_;
-  atomic_int id_ = 0;
-  atomic_int cid_ = -1;
+  atomic_int id_{0};
+  atomic_int cid_{-1};
   string message_;
 };
 

httplib.h

@@ -804,6 +804,11 @@ enum class Error {
   Compression,
 };
 
+inline std::ostream &operator<<(std::ostream &os, const Error &obj) {
+  os << static_cast<std::underlying_type<Error>::type>(obj);
+  return os;
+}
+
 class Result {
 public:
   Result(std::unique_ptr<Response> &&res, Error err,
@@ -999,6 +1004,8 @@ public:
   void set_keep_alive(bool on);
   void set_follow_location(bool on);
 
+  void set_url_encode(bool on);
+
   void set_compress(bool on);
 
   void set_decompress(bool on);
@@ -1043,10 +1050,6 @@ protected:
   void shutdown_socket(Socket &socket);
   void close_socket(Socket &socket);
 
-  // Similar to shutdown_ssl and close_socket, this should NOT be called
-  // concurrently with a DIFFERENT thread sending requests from the socket
-  void lock_socket_and_shutdown_and_close();
-
   bool process_request(Stream &strm, Request &req, Response &res,
                        bool close_connection, Error &error);
 
@@ -1095,6 +1098,8 @@ protected:
   bool keep_alive_ = false;
   bool follow_location_ = false;
 
+  bool url_encode_ = true;
+
   int address_family_ = AF_UNSPEC;
   bool tcp_nodelay_ = CPPHTTPLIB_TCP_NODELAY;
   SocketOptions socket_options_ = nullptr;
@@ -1312,6 +1317,8 @@ public:
   void set_keep_alive(bool on);
   void set_follow_location(bool on);
 
+  void set_url_encode(bool on);
+
   void set_compress(bool on);
 
   void set_decompress(bool on);
@@ -1401,6 +1408,7 @@ public:
 private:
   bool create_and_connect_socket(Socket &socket, Error &error) override;
   void shutdown_ssl(Socket &socket, bool shutdown_gracefully) override;
+  void shutdown_ssl_impl(Socket &socket, bool shutdown_socket);
 
   bool process_socket(const Socket &socket,
                       std::function<bool(Stream &strm)> callback) override;
@@ -1612,6 +1620,7 @@ inline std::string encode_query_param(const std::string &value) {
 
 inline std::string encode_url(const std::string &s) {
   std::string result;
+  result.reserve(s.size());
 
   for (size_t i = 0; s[i]; i++) {
     switch (s[i]) {
@@ -2223,45 +2232,45 @@ inline socket_t create_client_socket(
     time_t write_timeout_usec, const std::string &intf, Error &error) {
   auto sock = create_socket(
       host, port, address_family, 0, tcp_nodelay, std::move(socket_options),
-      [&](socket_t sock, struct addrinfo &ai) -> bool {
+      [&](socket_t sock2, struct addrinfo &ai) -> bool {
         if (!intf.empty()) {
 #ifdef USE_IF2IP
           auto ip = if2ip(intf);
           if (ip.empty()) { ip = intf; }
-          if (!bind_ip_address(sock, ip.c_str())) {
+          if (!bind_ip_address(sock2, ip.c_str())) {
             error = Error::BindIPAddress;
             return false;
           }
 #endif
         }
 
-        set_nonblocking(sock, true);
+        set_nonblocking(sock2, true);
 
         auto ret =
-            ::connect(sock, ai.ai_addr, static_cast<socklen_t>(ai.ai_addrlen));
+            ::connect(sock2, ai.ai_addr, static_cast<socklen_t>(ai.ai_addrlen));
 
         if (ret < 0) {
           if (is_connection_error() ||
-              !wait_until_socket_is_ready(sock, connection_timeout_sec,
+              !wait_until_socket_is_ready(sock2, connection_timeout_sec,
                                           connection_timeout_usec)) {
             error = Error::Connection;
             return false;
           }
         }
 
-        set_nonblocking(sock, false);
+        set_nonblocking(sock2, false);
 
         {
           timeval tv;
           tv.tv_sec = static_cast<long>(read_timeout_sec);
           tv.tv_usec = static_cast<decltype(tv.tv_usec)>(read_timeout_usec);
-          setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (char *)&tv, sizeof(tv));
+          setsockopt(sock2, SOL_SOCKET, SO_RCVTIMEO, (char *)&tv, sizeof(tv));
         }
         {
           timeval tv;
           tv.tv_sec = static_cast<long>(write_timeout_sec);
           tv.tv_usec = static_cast<decltype(tv.tv_usec)>(write_timeout_usec);
-          setsockopt(sock, SOL_SOCKET, SO_SNDTIMEO, (char *)&tv, sizeof(tv));
+          setsockopt(sock2, SOL_SOCKET, SO_SNDTIMEO, (char *)&tv, sizeof(tv));
         }
 
         error = Error::Success;
@@ -2551,7 +2560,7 @@ public:
 
     std::array<char, CPPHTTPLIB_COMPRESSION_BUFSIZ> buff{};
     do {
-      strm_.avail_out = buff.size();
+      strm_.avail_out = static_cast<uInt>(buff.size());
       strm_.next_out = reinterpret_cast<Bytef *>(buff.data());
 
       ret = deflate(&strm_, flush);
@@ -2602,7 +2611,7 @@ public:
 
     std::array<char, CPPHTTPLIB_COMPRESSION_BUFSIZ> buff{};
     while (strm_.avail_in > 0) {
-      strm_.avail_out = buff.size();
+      strm_.avail_out = static_cast<uInt>(buff.size());
       strm_.next_out = reinterpret_cast<Bytef *>(buff.data());
 
       ret = inflate(&strm_, Z_NO_FLUSH);
@@ -2939,8 +2948,8 @@ bool prepare_content_receiver(T &x, int &status,
         ContentReceiverWithProgress out = [&](const char *buf, size_t n,
                                               uint64_t off, uint64_t len) {
           return decompressor->decompress(buf, n,
-                                          [&](const char *buf, size_t n) {
-                                            return receiver(buf, n, off, len);
+                                          [&](const char *buf2, size_t n2) {
+                                            return receiver(buf2, n2, off, len);
                                           });
         };
         return callback(std::move(out));
@@ -3110,9 +3119,7 @@ write_content_chunked(Stream &strm, const ContentProvider &content_provider,
           // Emit chunked response header and footer for each chunk
           auto chunk =
               from_i_to_hex(payload.size()) + "\r\n" + payload + "\r\n";
-          if (!write_data(strm, chunk.data(), chunk.size())) {
-            ok = false;
-          }
+          if (!write_data(strm, chunk.data(), chunk.size())) { ok = false; }
         }
       } else {
         ok = false;
@@ -5248,7 +5255,11 @@ inline ClientImpl::ClientImpl(const std::string &host, int port,
       host_and_port_(host_ + ":" + std::to_string(port_)),
       client_cert_path_(client_cert_path), client_key_path_(client_key_path) {}
 
-inline ClientImpl::~ClientImpl() { lock_socket_and_shutdown_and_close(); }
+inline ClientImpl::~ClientImpl() {
+  std::lock_guard<std::mutex> guard(socket_mutex_);
+  shutdown_socket(socket_);
+  close_socket(socket_);
+}
 
 inline bool ClientImpl::is_valid() const { return true; }
 
@@ -5269,6 +5280,8 @@ inline void ClientImpl::copy_settings(const ClientImpl &rhs) {
 #endif
   keep_alive_ = rhs.keep_alive_;
   follow_location_ = rhs.follow_location_;
+  url_encode_ = rhs.url_encode_;
+  address_family_ = rhs.address_family_;
   tcp_nodelay_ = rhs.tcp_nodelay_;
   socket_options_ = rhs.socket_options_;
   compress_ = rhs.compress_;
@@ -5344,13 +5357,6 @@ inline void ClientImpl::close_socket(Socket &socket) {
   socket.sock = INVALID_SOCKET;
 }
 
-inline void ClientImpl::lock_socket_and_shutdown_and_close() {
-  std::lock_guard<std::mutex> guard(socket_mutex_);
-  shutdown_ssl(socket_, true);
-  shutdown_socket(socket_);
-  close_socket(socket_);
-}
-
 inline bool ClientImpl::read_response_line(Stream &strm, const Request &req,
                                            Response &res) {
   std::array<char, 2048> buf;
@@ -5696,7 +5702,7 @@ inline bool ClientImpl::write_request(Stream &strm, Request &req,
   {
     detail::BufferStream bstrm;
 
-    const auto &path = detail::encode_url(req.path);
+    const auto &path = url_encode_ ? detail::encode_url(req.path) : req.path;
     bstrm.write_format("%s %s HTTP/1.1\r\n", req.method.c_str(), path.c_str());
 
     detail::write_headers(bstrm, req.headers);
@@ -5899,7 +5905,10 @@ inline bool ClientImpl::process_request(Stream &strm, Request &req,
     // mutex during the process. It would be a bug to call it from a different
     // thread since it's a thread-safety issue to do these things to the socket
     // if another thread is using the socket.
-    lock_socket_and_shutdown_and_close();
+    std::lock_guard<std::mutex> guard(socket_mutex_);
+    shutdown_ssl(socket_, true);
+    shutdown_socket(socket_);
+    close_socket(socket_);
   }
 
   // Log
@@ -6425,6 +6434,8 @@ inline void ClientImpl::set_keep_alive(bool on) { keep_alive_ = on; }
 
 inline void ClientImpl::set_follow_location(bool on) { follow_location_ = on; }
 
+inline void ClientImpl::set_url_encode(bool on) { url_encode_ = on; }
+
 inline void ClientImpl::set_default_headers(Headers headers) {
   default_headers_ = std::move(headers);
 }
@@ -6657,7 +6668,12 @@ inline ssize_t SSLSocketStream::read(char *ptr, size_t size) {
     auto ret = SSL_read(ssl_, ptr, static_cast<int>(size));
     if (ret < 0) {
       auto err = SSL_get_error(ssl_, ret);
+#ifdef _WIN32
+      while (err == SSL_ERROR_WANT_READ ||
+             err == SSL_ERROR_SYSCALL && WSAGetLastError() == WSAETIMEDOUT) {
+#else
       while (err == SSL_ERROR_WANT_READ) {
+#endif
         if (SSL_pending(ssl_) > 0) {
           return SSL_read(ssl_, ptr, static_cast<int>(size));
         } else if (is_readable()) {
@@ -6845,7 +6861,7 @@ inline SSLClient::~SSLClient() {
   // Make sure to shut down SSL since shutdown_ssl will resolve to the
   // base function rather than the derived function once we get to the
   // base class destructor, and won't free the SSL (causing a leak).
-  SSLClient::shutdown_ssl(socket_, true);
+  shutdown_ssl_impl(socket_, true);
 }
 
 inline bool SSLClient::is_valid() const { return ctx_; }
@@ -7024,6 +7040,10 @@ inline bool SSLClient::initialize_ssl(Socket &socket, Error &error) {
 }
 
 inline void SSLClient::shutdown_ssl(Socket &socket, bool shutdown_gracefully) {
+  shutdown_ssl_impl(socket, shutdown_gracefully);
+}
+
+inline void SSLClient::shutdown_ssl_impl(Socket &socket, bool shutdown_gracefully) {
   if (socket.sock == INVALID_SOCKET) {
     assert(socket.ssl == nullptr);
     return;
@@ -7553,6 +7573,8 @@ inline void Client::set_follow_location(bool on) {
   cli_->set_follow_location(on);
 }
 
+inline void Client::set_url_encode(bool on) { cli_->set_url_encode(on); }
+
 inline void Client::set_compress(bool on) { cli_->set_compress(on); }
 
 inline void Client::set_decompress(bool on) { cli_->set_decompress(on); }

test/test.cc

@@ -7,6 +7,7 @@
 #include <future>
 #include <stdexcept>
 #include <thread>
+#include <sstream>
 
 #define SERVER_CERT_FILE "./cert.pem"
 #define SERVER_CERT2_FILE "./cert2.pem"
@@ -547,6 +548,23 @@ TEST(ConnectionErrorTest, InvalidHost2) {
   EXPECT_EQ(Error::Connection, res.error());
 }
 
+TEST(ConnectionErrorTest, InvalidHostCheckResultErrorToString) {
+  auto host = "httpbin.org/";
+
+#ifdef CPPHTTPLIB_OPENSSL_SUPPORT
+  SSLClient cli(host);
+#else
+  Client cli(host);
+#endif
+  cli.set_connection_timeout(std::chrono::seconds(2));
+
+  auto res = cli.Get("/");
+  ASSERT_TRUE(!res);
+  stringstream s;
+  s << "error code: " << res.error();
+  EXPECT_EQ("error code: 2", s.str());
+}
+
 TEST(ConnectionErrorTest, InvalidPort) {
   auto host = "localhost";
   auto port = 44380;
@@ -947,6 +965,42 @@ TEST(RedirectFromPageWithContent, Redirect) {
 
 #endif
 
+TEST(PathUrlEncodeTest, PathUrlEncode) {
+  Server svr;
+
+  svr.Get("/foo", [](const Request & req, Response &res) {
+    auto a = req.params.find("a");
+    if (a != req.params.end()) {
+      res.set_content((*a).second, "text/plain");
+      res.status = 200;
+    } else {
+      res.status = 400;
+    }
+  });
+
+  auto thread = std::thread([&]() { svr.listen(HOST, PORT); });
+
+  // Give GET time to get a few messages.
+  std::this_thread::sleep_for(std::chrono::seconds(1));
+
+  {
+    Client cli(HOST, PORT);
+    cli.set_url_encode(false);
+
+    auto res = cli.Get("/foo?a=explicitly+encoded");
+    ASSERT_TRUE(res);
+    EXPECT_EQ(200, res->status);
+    // This expects it back with a space, as the `+` won't have been
+    // url-encoded, and server-side the params get decoded turning `+`
+    // into spaces.
+    EXPECT_EQ("explicitly encoded", res->body);
+  }
+
+  svr.stop();
+  thread.join();
+  ASSERT_FALSE(svr.is_running());
+}
+
 TEST(BindServerTest, BindDualStack) {
   Server svr;