Movable HTTPClient and fixing WiFiClient copy (#8237)

- =default for default ctor, destructor, move ctor and the assignment move
- use `std::unique_ptr<WiFiClient>` instead of raw pointer to the client
- implement `virtual std::unique_ptr<WiFiClient> WiFiClient::clone()` to safely copy the WiFiClientSecure instance, without accidentally slicing it (i.e. using the pointer with incorrect type, calling base WiFiClient virtual methods)
- replace headers pointer array with `std::unique_ptr<T[]>` to simplify the move operations
- substitute userAgent with the default one when it is empty
(may be a subject to change though, b/c now there is a global static `String`)

Allow HTTPClient to be placed inside of movable classes (e.g. std::optional, requested in the linked issue) or to be returned from functions. Class logic stays as-is, only the underlying member types are changed.

Notice that WiFiClient connection object is now copied, and the internal ClientContext will be preserved even after the original WiFiClient object was destroyed.

replaces #8236
resolves #8231
and, possibly #5734

Author: mcspr

Diff for: libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

@@ -28,6 +28,16 @@
 #include <StreamDev.h>
 #include <base64.h>
 
+// per https://github.com/esp8266/Arduino/issues/8231
+// make sure HTTPClient can be utilized as a movable class member
+static_assert(std::is_default_constructible_v<HTTPClient>, "");
+static_assert(!std::is_copy_constructible_v<HTTPClient>, "");
+static_assert(std::is_move_constructible_v<HTTPClient>, "");
+static_assert(std::is_move_assignable_v<HTTPClient>, "");
+
+static const char defaultUserAgentPstr[] PROGMEM = "ESP8266HTTPClient";
+const String HTTPClient::defaultUserAgent = defaultUserAgentPstr;
+
 static int StreamReportToHttpClientReport (Stream::Report streamSendError)
 {
     switch (streamSendError)
@@ -41,27 +51,6 @@ static int StreamReportToHttpClientReport (Stream::Report streamSendError)
     return 0; // never reached, keep gcc quiet
 }
 
-/**
- * constructor
- */
-HTTPClient::HTTPClient()
-    : _client(nullptr), _userAgent(F("ESP8266HTTPClient"))
-{
-}
-
-/**
- * destructor
- */
-HTTPClient::~HTTPClient()
-{
-    if(_client) {
-        _client->stop();
-    }
-    if(_currentHeaders) {
-        delete[] _currentHeaders;
-    }
-}
-
 void HTTPClient::clear()
 {
     _returnCode = 0;
@@ -80,8 +69,6 @@ void HTTPClient::clear()
  * @return success bool
  */
 bool HTTPClient::begin(WiFiClient &client, const String& url) {
-    _client = &client;
-
     // check for : (http: or https:)
     int index = url.indexOf(':');
     if(index < 0) {
@@ -97,6 +84,8 @@ bool HTTPClient::begin(WiFiClient &client, const String& url) {
     }
 
     _port = (protocol == "https" ? 443 : 80);
+    _client = client.clone();
+
     return beginInternal(url, protocol.c_str());
 }
 
@@ -112,7 +101,7 @@ bool HTTPClient::begin(WiFiClient &client, const String& url) {
  */
 bool HTTPClient::begin(WiFiClient &client, const String& host, uint16_t port, const String& uri, bool https)
 {
-    _client = &client;
+    _client = client.clone();
 
      clear();
     _host = host;
@@ -462,7 +451,7 @@ int HTTPClient::sendRequest(const char * type, const uint8_t * payload, size_t s
         }
 
         // transfer all of it, with send-timeout
-        if (size && StreamConstPtr(payload, size).sendAll(_client) != size)
+        if (size && StreamConstPtr(payload, size).sendAll(_client.get()) != size)
             return returnError(HTTPC_ERROR_SEND_PAYLOAD_FAILED);
 
         // handle Server Response (Header)
@@ -563,7 +552,7 @@ int HTTPClient::sendRequest(const char * type, Stream * stream, size_t size)
     }
 
     // transfer all of it, with timeout
-    size_t transferred = stream->sendSize(_client, size);
+    size_t transferred = stream->sendSize(_client.get(), size);
     if (transferred != size)
     {
         DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] short write, asked for %d but got %d failed.\n", size, transferred);
@@ -613,7 +602,7 @@ WiFiClient& HTTPClient::getStream(void)
 WiFiClient* HTTPClient::getStreamPtr(void)
 {
     if(connected()) {
-        return _client;
+        return _client.get();
     }
 
     DEBUG_HTTPCLIENT("[HTTP-Client] getStreamPtr: not connected\n");
@@ -818,10 +807,7 @@ void HTTPClient::addHeader(const String& name, const String& value, bool first,
 void HTTPClient::collectHeaders(const char* headerKeys[], const size_t headerKeysCount)
 {
     _headerKeysCount = headerKeysCount;
-    if(_currentHeaders) {
-        delete[] _currentHeaders;
-    }
-    _currentHeaders = new RequestArgument[_headerKeysCount];
+    _currentHeaders = std::make_unique<RequestArgument[]>(_headerKeysCount);
     for(size_t i = 0; i < _headerKeysCount; i++) {
         _currentHeaders[i].key = headerKeys[i];
     }
@@ -963,7 +949,7 @@ bool HTTPClient::sendHeader(const char * type)
     DEBUG_HTTPCLIENT("[HTTP-Client] sending request header\n-----\n%s-----\n", header.c_str());
 
     // transfer all of it, with timeout
-    return StreamConstPtr(header).sendAll(_client) == header.length();
+    return StreamConstPtr(header).sendAll(_client.get()) == header.length();
 }
 
 /**

Diff for: libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h

@@ -26,11 +26,12 @@
 #ifndef ESP8266HTTPClient_H_
 #define ESP8266HTTPClient_H_
 
-#include <memory>
 #include <Arduino.h>
 #include <StreamString.h>
 #include <WiFiClient.h>
 
+#include <memory>
+
 #ifdef DEBUG_ESP_HTTP_CLIENT
 #ifdef DEBUG_ESP_PORT
 #define DEBUG_HTTPCLIENT(fmt, ...) DEBUG_ESP_PORT.printf_P( (PGM_P)PSTR(fmt), ## __VA_ARGS__ )
@@ -151,13 +152,12 @@ typedef std::unique_ptr<TransportTraits> TransportTraitsPtr;
 class HTTPClient
 {
 public:
-    HTTPClient();
-    ~HTTPClient();
+    HTTPClient() = default;
+    ~HTTPClient() = default;
+    HTTPClient(HTTPClient&&) = default;
+    HTTPClient& operator=(HTTPClient&&) = default;
 
-/*
- * Since both begin() functions take a reference to client as a parameter, you need to 
- * ensure the client object lives the entire time of the HTTPClient
- */
+    // Note that WiFiClient's underlying connection *will* be captured
     bool begin(WiFiClient &client, const String& url);
     bool begin(WiFiClient &client, const String& host, uint16_t port, const String& uri = "/", bool https = false);
 
@@ -235,7 +235,15 @@ class HTTPClient
     int handleHeaderResponse();
     int writeToStreamDataBlock(Stream * stream, int len);
 
-    WiFiClient* _client;
+    // The common pattern to use the class is to
+    // {
+    //     WiFiClient socket;
+    //     HTTPClient http;
+    //     http.begin(socket, "http://blahblah");
+    // }
+    // Make sure it's not possible to break things in an opposite direction
+
+    std::unique_ptr<WiFiClient> _client;
 
     /// request handling
     String _host;
@@ -247,12 +255,14 @@ class HTTPClient
     String _uri;
     String _protocol;
     String _headers;
-    String _userAgent;
     String _base64Authorization;
 
+    static const String defaultUserAgent;
+    String _userAgent = defaultUserAgent;
+
     /// Response handling
-    RequestArgument* _currentHeaders = nullptr;
-    size_t           _headerKeysCount = 0;
+    std::unique_ptr<RequestArgument[]> _currentHeaders;
+    size_t _headerKeysCount = 0;
 
     int _returnCode = 0;
     int _size = -1;
@@ -264,6 +274,4 @@ class HTTPClient
     std::unique_ptr<StreamString> _payload;
 };
 
-
-
 #endif /* ESP8266HTTPClient_H_ */

Diff for: libraries/ESP8266WiFi/src/WiFiClient.cpp

@@ -101,6 +101,10 @@ WiFiClient::~WiFiClient()
         _client->unref();
 }
 
+std::unique_ptr<WiFiClient> WiFiClient::clone() const {
+    return std::make_unique<WiFiClient>(*this);
+}
+
 WiFiClient::WiFiClient(const WiFiClient& other)
 {
     _client = other._client;

Diff for: libraries/ESP8266WiFi/src/WiFiClient.h

@@ -52,6 +52,18 @@ class WiFiClient : public Client, public SList<WiFiClient> {
   WiFiClient(const WiFiClient&);
   WiFiClient& operator=(const WiFiClient&);
 
+  // b/c this is both a real class and a virtual parent of the secure client, make sure
+  // there's a safe way to copy from the pointer without 'slicing' it; i.e. only the base
+  // portion of a derived object will be copied, and the polymorphic behavior will be corrupted. 
+  //
+  // this class still implements the copy and assignment though, so this is not yet enforced
+  // (but, *should* be inside the Core itself, see httpclient & server)
+  //
+  // ref.
+  // - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-copy-virtual
+  // - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-copy
+  virtual std::unique_ptr<WiFiClient> clone() const;
+
   virtual uint8_t status();
   virtual int connect(IPAddress ip, uint16_t port) override;
   virtual int connect(const char *host, uint16_t port) override;

Diff for: libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h

@@ -39,6 +39,13 @@ class WiFiClientSecureCtx : public WiFiClient {
 
     WiFiClientSecureCtx& operator=(const WiFiClientSecureCtx&) = delete;
 
+    // TODO: usage is invalid b/c of deleted copy, but this will only trigger an error when it is actually used by something
+    // TODO: don't remove just yet to avoid including the WiFiClient default implementation and unintentionally causing
+    //       a 'slice' that this method tries to avoid in the first place
+    std::unique_ptr<WiFiClient> clone() const override {
+        return std::unique_ptr<WiFiClient>(new WiFiClientSecureCtx(*this));
+    }
+
     int connect(IPAddress ip, uint16_t port) override;
     int connect(const String& host, uint16_t port) override;
     int connect(const char* name, uint16_t port) override;
@@ -231,13 +238,23 @@ class WiFiClientSecure : public WiFiClient {
   // Instead, all virtual functions call their counterpart in "WiFiClientecureCtx* _ctx"
   //          which also derives from WiFiClient (this parent is the one which is eventually used)
 
+  // TODO: notice that this complicates the implementation by having two distinct ways the client connection is managed, consider:
+  // - implementing the secure connection details in the ClientContext
+  //   (i.e. delegate the write & read functions there)
+  // - simplify the inheritance chain by implementing base wificlient class and inherit the original wificlient and wificlientsecure from it
+  // - abstract internals so it's possible to seamlessly =default copy and move with the instance *without* resorting to manual copy and initialization of each member
+
+  // TODO: prefer implementing virtual overrides in the .cpp (or, at least one of them)
+
   public:
 
     WiFiClientSecure():_ctx(new WiFiClientSecureCtx()) { _owned = _ctx.get(); }
     WiFiClientSecure(const WiFiClientSecure &rhs): WiFiClient(), _ctx(rhs._ctx) { if (_ctx) _owned = _ctx.get(); }
     ~WiFiClientSecure() override { _ctx = nullptr; }
 
-    WiFiClientSecure& operator=(const WiFiClientSecure&) = default; // The shared-ptrs handle themselves automatically
+    WiFiClientSecure& operator=(const WiFiClientSecure&) = default;
+
+    std::unique_ptr<WiFiClient> clone() const override { return std::unique_ptr<WiFiClient>(new WiFiClientSecure(*this)); }
 
     uint8_t status() override { return _ctx->status(); }
     int connect(IPAddress ip, uint16_t port) override { return _ctx->connect(ip, port); }