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 instance *will* be captured by the internal handler.
     bool begin(WiFiClient &client, const String& url);
     bool begin(WiFiClient &client, const String& host, uint16_t port, const String& uri = "/", bool https = false);
 
@@ -226,6 +226,15 @@ class HTTPClient
         String value;
     };
 
+    // TODO: the common pattern to use the class is to
+    // {
+    //     WiFiClient socket;
+    //     HTTPClient http;
+    //     http.begin(socket, "http://blahblah");
+    // }
+    // in case wificlient supports seamless ref() / unref() of the underlying connection
+    // for both wificlient and wificlientsecure, this may be removed in favour of that approach.
+
     bool beginInternal(const String& url, const char* expectedProtocol);
     void disconnect(bool preserveClient = false);
     void clear();
@@ -235,7 +244,7 @@ class HTTPClient
     int handleHeaderResponse();
     int writeToStreamDataBlock(Stream * stream, int len);
 
-    WiFiClient* _client;
+    std::unique_ptr<WiFiClient> _client;
 
     /// request handling
     String _host;
@@ -247,12 +256,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 +275,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,12 @@ class WiFiClient : public Client, public SList<WiFiClient> {
   WiFiClient(const WiFiClient&);
   WiFiClient& operator=(const WiFiClient&);
 
+  // b/c wificlient is both a real class and the virtual base for secure client,
+  // make sure there's a safe way to clone the object without accidentally 'slicing' it
+  // (...which will happen when using normal copy ctor with the dereferenced pointer...)
+  // TODO: but, actually, implement secure handlers in the ClientContext instead, so normal copy works
+  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,12 @@ class WiFiClientSecureCtx : public WiFiClient {
 
     WiFiClientSecureCtx& operator=(const WiFiClientSecureCtx&) = delete;
 
+    // TODO: usage is invalid invalid, but this will trigger an error only when
+    // it is actually used by something and `=delete`ed ctor above is accessed
+    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;
@@ -239,6 +245,8 @@ class WiFiClientSecure : public WiFiClient {
 
     WiFiClientSecure& operator=(const WiFiClientSecure&) = default; // The shared-ptrs handle themselves automatically
 
+    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); }
     int connect(const String& host, uint16_t port) override { return _ctx->connect(host, port); }