Movable HTTPClient and fixing WiFiClient copy

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();
 }
 
 /**

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_ */

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;

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;

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); }