diff --git a/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp b/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp index b98edcccc0..b35892e748 100644 --- a/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp +++ b/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp @@ -28,6 +28,16 @@ #include #include +// 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, ""); +static_assert(!std::is_copy_constructible_v, ""); +static_assert(std::is_move_constructible_v, ""); +static_assert(std::is_move_assignable_v, ""); + +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(_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 --git a/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h b/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h index 4b9015c9e5..9e3aef9ecd 100644 --- a/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h +++ b/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h @@ -26,11 +26,12 @@ #ifndef ESP8266HTTPClient_H_ #define ESP8266HTTPClient_H_ -#include #include #include #include +#include + #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 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 _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 _currentHeaders; + size_t _headerKeysCount = 0; int _returnCode = 0; int _size = -1; @@ -264,6 +274,4 @@ class HTTPClient std::unique_ptr _payload; }; - - #endif /* ESP8266HTTPClient_H_ */ diff --git a/libraries/ESP8266WiFi/src/WiFiClient.cpp b/libraries/ESP8266WiFi/src/WiFiClient.cpp index decc7d7ac0..74abc23655 100644 --- a/libraries/ESP8266WiFi/src/WiFiClient.cpp +++ b/libraries/ESP8266WiFi/src/WiFiClient.cpp @@ -101,6 +101,10 @@ WiFiClient::~WiFiClient() _client->unref(); } +std::unique_ptr WiFiClient::clone() const { + return std::make_unique(*this); +} + WiFiClient::WiFiClient(const WiFiClient& other) { _client = other._client; diff --git a/libraries/ESP8266WiFi/src/WiFiClient.h b/libraries/ESP8266WiFi/src/WiFiClient.h index e7afcaa538..5065622cc2 100644 --- a/libraries/ESP8266WiFi/src/WiFiClient.h +++ b/libraries/ESP8266WiFi/src/WiFiClient.h @@ -52,6 +52,18 @@ class WiFiClient : public Client, public SList { 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 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 --git a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h index 73dc9e7337..8a77648d90 100644 --- a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h +++ b/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 clone() const override { + return std::unique_ptr(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 clone() const override { return std::unique_ptr(new WiFiClientSecure(*this)); } uint8_t status() override { return _ctx->status(); } int connect(IPAddress ip, uint16_t port) override { return _ctx->connect(ip, port); }