From ec461578e65f7b6c0b4a335e8cf2622903ea00f2 Mon Sep 17 00:00:00 2001 From: David Schroeder Date: Sun, 26 Feb 2017 12:48:28 -0500 Subject: [PATCH 1/3] Rework WiFiClient Rework WiFiClient to correct error where making a copy of a WiFiClient object resulted in the socket being closed prematurely. Added loop and select to write to handle/prevent EAGAIN errors. --- libraries/WiFi/src/WiFiClient.cpp | 132 +++++++++++++++++++++++------- libraries/WiFi/src/WiFiClient.h | 49 +++++++++-- 2 files changed, 142 insertions(+), 39 deletions(-) diff --git a/libraries/WiFi/src/WiFiClient.cpp b/libraries/WiFi/src/WiFiClient.cpp index ec92acc145c..30fd9981ae2 100644 --- a/libraries/WiFi/src/WiFiClient.cpp +++ b/libraries/WiFi/src/WiFiClient.cpp @@ -26,12 +26,21 @@ #undef write #undef read -WiFiClient::WiFiClient():sockfd(-1),_connected(false),next(NULL) +#define WIFI_CLIENT_SELECT_TIMEOUT_US (100000) + +WiFiClientSocketHandle::~WiFiClientSocketHandle() +{ + close(sockfd); +} + +WiFiClient::WiFiClient():_connected(false),next(NULL) { + clientSocketHandle = NULL; } -WiFiClient::WiFiClient(int fd):sockfd(fd),_connected(true),next(NULL) +WiFiClient::WiFiClient(int fd):_connected(true),next(NULL) { + clientSocketHandle = new WiFiClientSocketHandle(fd); } WiFiClient::~WiFiClient() @@ -42,27 +51,47 @@ WiFiClient::~WiFiClient() WiFiClient & WiFiClient::operator=(const WiFiClient &other) { stop(); - sockfd = other.sockfd; + clientSocketHandle = other.clientSocketHandle; + if (clientSocketHandle != NULL) { + clientSocketHandle->ref(); + } _connected = other._connected; return *this; } +WiFiClient::WiFiClient(const WiFiClient &other) +{ + clientSocketHandle = other.clientSocketHandle; + if (clientSocketHandle != NULL) { + clientSocketHandle->ref(); + } + _connected = other._connected; + next = other.next; +} + void WiFiClient::stop() { - if(_connected && sockfd >= 0) { - close(sockfd); - sockfd = -1; - _connected = false; + if (clientSocketHandle && clientSocketHandle->unref() == 0) { + delete clientSocketHandle; } + clientSocketHandle = NULL; + _connected = false; } int WiFiClient::connect(IPAddress ip, uint16_t port) { - sockfd = socket(AF_INET, SOCK_STREAM, 0); + int sockfd = socket(AF_INET, SOCK_STREAM, 0); if (sockfd < 0) { log_e("socket: %d", errno); return 0; } + + //If there is an exisiting socket, unreference it + if (clientSocketHandle && clientSocketHandle->unref() == 0) { + delete clientSocketHandle; + } + clientSocketHandle = new WiFiClientSocketHandle(sockfd); + uint32_t ip_addr = ip; struct sockaddr_in serveraddr; bzero((char *) &serveraddr, sizeof(serveraddr)); @@ -72,8 +101,8 @@ int WiFiClient::connect(IPAddress ip, uint16_t port) int res = lwip_connect_r(sockfd, (struct sockaddr*)&serveraddr, sizeof(serveraddr)); if (res < 0) { log_e("lwip_connect_r: %d", errno); - close(sockfd); - sockfd = -1; + delete clientSocketHandle; + clientSocketHandle = NULL; return 0; } _connected = true; @@ -93,7 +122,10 @@ int WiFiClient::connect(const char *host, uint16_t port) int WiFiClient::setSocketOption(int option, char* value, size_t len) { - int res = setsockopt(sockfd, SOL_SOCKET, option, value, len); + if (clientSocketHandle == NULL) { + return -1; + } + int res = setsockopt(fd(), SOL_SOCKET, option, value, len); if(res < 0) { log_e("%d", errno); } @@ -113,8 +145,12 @@ int WiFiClient::setTimeout(uint32_t seconds) int WiFiClient::setOption(int option, int *value) { - int res = setsockopt(sockfd, IPPROTO_TCP, option, (char *)value, sizeof(int)); - if(res < 0) { + if (clientSocketHandle == NULL) { + return -1; + } + int res = setsockopt(fd(), IPPROTO_TCP, option, (char *) value, + sizeof(int)); + if (res < 0) { log_e("%d", errno); } return res; @@ -122,8 +158,11 @@ int WiFiClient::setOption(int option, int *value) int WiFiClient::getOption(int option, int *value) { + if (clientSocketHandle == NULL) { + return -1; + } size_t size = sizeof(int); - int res = getsockopt(sockfd, IPPROTO_TCP, option, (char *)value, &size); + int res = getsockopt(fd(), IPPROTO_TCP, option, (char *)value, &size); if(res < 0) { log_e("%d", errno); } @@ -160,24 +199,55 @@ int WiFiClient::read() size_t WiFiClient::write(const uint8_t *buf, size_t size) { - if(!_connected) { + int res =0; + bool retry = true; + + if (!_connected || clientSocketHandle == NULL) { return 0; } - int res = send(sockfd, (void*)buf, size, MSG_DONTWAIT); - if(res < 0) { - log_e("%d", errno); - stop(); - res = 0; + + int sockfd = fd(); + + while (retry) { + //use select to make sure the socket is ready for writing + fd_set set; + struct timeval tv; + FD_ZERO(&set); /* empties the set */ + FD_SET(sockfd, &set); /* adds FD to the set */ + tv.tv_sec = 0; + tv.tv_usec = WIFI_CLIENT_SELECT_TIMEOUT_US; + + if (select(sockfd + 1, NULL, &set, NULL, &tv) < 0) { + return 0; + } + + if (FD_ISSET(sockfd, &set)) { + res = send(sockfd, (void*) buf, size, MSG_DONTWAIT); + if (res < 0) { + log_e("%d", errno); + Serial.print("Write Error: "); + Serial.println(errno); + if (errno != EAGAIN) { + //if resource was busy, can try again, otherwise give up + stop(); + res = 0; + retry = false; + } + } else { + //completed successfully + retry = false; + } + } } return res; } int WiFiClient::read(uint8_t *buf, size_t size) { - if(!available()) { + if (!available() || clientSocketHandle == NULL) { return -1; } - int res = recv(sockfd, buf, size, MSG_DONTWAIT); + int res = recv(fd(), buf, size, MSG_DONTWAIT); if(res < 0 && errno != EWOULDBLOCK) { log_e("%d", errno); stop(); @@ -187,11 +257,11 @@ int WiFiClient::read(uint8_t *buf, size_t size) int WiFiClient::available() { - if(!_connected) { + if (!_connected || clientSocketHandle == NULL) { return 0; } int count; - int res = ioctl(sockfd, FIONREAD, &count); + int res = ioctl(fd(), FIONREAD, &count); if(res < 0) { log_e("%d", errno); stop(); @@ -207,7 +277,7 @@ uint8_t WiFiClient::connected() return _connected; } -IPAddress WiFiClient::remoteIP(int fd) +IPAddress WiFiClient::remoteIP(int fd) const { struct sockaddr_storage addr; socklen_t len = sizeof addr; @@ -216,7 +286,7 @@ IPAddress WiFiClient::remoteIP(int fd) return IPAddress((uint32_t)(s->sin_addr.s_addr)); } -uint16_t WiFiClient::remotePort(int fd) +uint16_t WiFiClient::remotePort(int fd) const { struct sockaddr_storage addr; socklen_t len = sizeof addr; @@ -225,17 +295,17 @@ uint16_t WiFiClient::remotePort(int fd) return ntohs(s->sin_port); } -IPAddress WiFiClient::remoteIP() +IPAddress WiFiClient::remoteIP() const { - return remoteIP(sockfd); + return remoteIP(fd()); } -uint16_t WiFiClient::remotePort() +uint16_t WiFiClient::remotePort() const { - return remotePort(sockfd); + return remotePort(fd()); } bool WiFiClient::operator==(const WiFiClient& rhs) { - return sockfd == rhs.sockfd && remotePort(sockfd) == remotePort(rhs.sockfd) && remoteIP(sockfd) == remoteIP(rhs.sockfd); + return clientSocketHandle == rhs.clientSocketHandle && remotePort() == rhs.remotePort() && remoteIP() == rhs.remoteIP(); } diff --git a/libraries/WiFi/src/WiFiClient.h b/libraries/WiFi/src/WiFiClient.h index b2e3e4500d7..2e2d925710a 100644 --- a/libraries/WiFi/src/WiFiClient.h +++ b/libraries/WiFi/src/WiFiClient.h @@ -20,20 +20,48 @@ #ifndef _WIFICLIENT_H_ #define _WIFICLIENT_H_ - #include "Arduino.h" #include "Client.h" +class WiFiClientSocketHandle { +private: + int sockfd; + int refCount; + +public: + WiFiClientSocketHandle(int fd):sockfd(fd), refCount(1) + { + } + ; + ~WiFiClientSocketHandle(); + + int ref() + { + return ++refCount; + } + + int unref() + { + return --refCount; + } + + int fd() + { + return sockfd; + } +}; + class WiFiClient : public Client { protected: - int sockfd; + WiFiClientSocketHandle *clientSocketHandle; bool _connected; public: WiFiClient *next; WiFiClient(); WiFiClient(int fd); + WiFiClient(const WiFiClient &other); ~WiFiClient(); int connect(IPAddress ip, uint16_t port); int connect(const char *host, uint16_t port); @@ -69,12 +97,15 @@ class WiFiClient : public Client return !this->operator==(rhs); }; - int fd() + int fd() const { - return sockfd; + if (clientSocketHandle == NULL) { + return -1; + } else { + return clientSocketHandle->fd(); + } } - IPAddress remoteIP(); - uint16_t remotePort(); + int setSocketOption(int option, char* value, size_t len); int setOption(int option, int *value); int getOption(int option, int *value); @@ -82,8 +113,10 @@ class WiFiClient : public Client int setNoDelay(bool nodelay); bool getNoDelay(); - IPAddress remoteIP(int fd); - uint16_t remotePort(int fd); + IPAddress remoteIP() const; + IPAddress remoteIP(int fd) const; + uint16_t remotePort() const; + uint16_t remotePort(int fd) const; //friend class WiFiServer; using Print::write; From 19e6b43795b6bb5b561ec4119a9fdb7d2ff5b6e1 Mon Sep 17 00:00:00 2001 From: David Schroeder Date: Mon, 27 Feb 2017 11:28:14 -0500 Subject: [PATCH 2/3] Rework WiFiClient to use shared_ptr Rework changes to utilize shared_ptr rather than manually maintaining reference count. Revert changes to write --- libraries/WiFi/src/WiFiClient.cpp | 91 +++++-------------------------- libraries/WiFi/src/WiFiClient.h | 20 ++----- 2 files changed, 18 insertions(+), 93 deletions(-) diff --git a/libraries/WiFi/src/WiFiClient.cpp b/libraries/WiFi/src/WiFiClient.cpp index 30fd9981ae2..20af39e35f1 100644 --- a/libraries/WiFi/src/WiFiClient.cpp +++ b/libraries/WiFi/src/WiFiClient.cpp @@ -26,7 +26,6 @@ #undef write #undef read -#define WIFI_CLIENT_SELECT_TIMEOUT_US (100000) WiFiClientSocketHandle::~WiFiClientSocketHandle() { @@ -35,12 +34,11 @@ WiFiClientSocketHandle::~WiFiClientSocketHandle() WiFiClient::WiFiClient():_connected(false),next(NULL) { - clientSocketHandle = NULL; } WiFiClient::WiFiClient(int fd):_connected(true),next(NULL) { - clientSocketHandle = new WiFiClientSocketHandle(fd); + clientSocketHandle.reset(new WiFiClientSocketHandle(fd)); } WiFiClient::~WiFiClient() @@ -52,28 +50,12 @@ WiFiClient & WiFiClient::operator=(const WiFiClient &other) { stop(); clientSocketHandle = other.clientSocketHandle; - if (clientSocketHandle != NULL) { - clientSocketHandle->ref(); - } _connected = other._connected; return *this; } -WiFiClient::WiFiClient(const WiFiClient &other) -{ - clientSocketHandle = other.clientSocketHandle; - if (clientSocketHandle != NULL) { - clientSocketHandle->ref(); - } - _connected = other._connected; - next = other.next; -} - void WiFiClient::stop() { - if (clientSocketHandle && clientSocketHandle->unref() == 0) { - delete clientSocketHandle; - } clientSocketHandle = NULL; _connected = false; } @@ -86,12 +68,6 @@ int WiFiClient::connect(IPAddress ip, uint16_t port) return 0; } - //If there is an exisiting socket, unreference it - if (clientSocketHandle && clientSocketHandle->unref() == 0) { - delete clientSocketHandle; - } - clientSocketHandle = new WiFiClientSocketHandle(sockfd); - uint32_t ip_addr = ip; struct sockaddr_in serveraddr; bzero((char *) &serveraddr, sizeof(serveraddr)); @@ -101,10 +77,10 @@ int WiFiClient::connect(IPAddress ip, uint16_t port) int res = lwip_connect_r(sockfd, (struct sockaddr*)&serveraddr, sizeof(serveraddr)); if (res < 0) { log_e("lwip_connect_r: %d", errno); - delete clientSocketHandle; - clientSocketHandle = NULL; + close(sockfd); return 0; } + clientSocketHandle.reset(new WiFiClientSocketHandle(sockfd)); _connected = true; return 1; } @@ -122,9 +98,6 @@ int WiFiClient::connect(const char *host, uint16_t port) int WiFiClient::setSocketOption(int option, char* value, size_t len) { - if (clientSocketHandle == NULL) { - return -1; - } int res = setsockopt(fd(), SOL_SOCKET, option, value, len); if(res < 0) { log_e("%d", errno); @@ -145,12 +118,8 @@ int WiFiClient::setTimeout(uint32_t seconds) int WiFiClient::setOption(int option, int *value) { - if (clientSocketHandle == NULL) { - return -1; - } - int res = setsockopt(fd(), IPPROTO_TCP, option, (char *) value, - sizeof(int)); - if (res < 0) { + int res = setsockopt(fd(), IPPROTO_TCP, option, (char *) value, sizeof(int)); + if(res < 0) { log_e("%d", errno); } return res; @@ -158,9 +127,6 @@ int WiFiClient::setOption(int option, int *value) int WiFiClient::getOption(int option, int *value) { - if (clientSocketHandle == NULL) { - return -1; - } size_t size = sizeof(int); int res = getsockopt(fd(), IPPROTO_TCP, option, (char *)value, &size); if(res < 0) { @@ -199,52 +165,21 @@ int WiFiClient::read() size_t WiFiClient::write(const uint8_t *buf, size_t size) { - int res =0; - bool retry = true; - - if (!_connected || clientSocketHandle == NULL) { + if(!_connected) { return 0; } - - int sockfd = fd(); - - while (retry) { - //use select to make sure the socket is ready for writing - fd_set set; - struct timeval tv; - FD_ZERO(&set); /* empties the set */ - FD_SET(sockfd, &set); /* adds FD to the set */ - tv.tv_sec = 0; - tv.tv_usec = WIFI_CLIENT_SELECT_TIMEOUT_US; - - if (select(sockfd + 1, NULL, &set, NULL, &tv) < 0) { - return 0; - } - - if (FD_ISSET(sockfd, &set)) { - res = send(sockfd, (void*) buf, size, MSG_DONTWAIT); - if (res < 0) { - log_e("%d", errno); - Serial.print("Write Error: "); - Serial.println(errno); - if (errno != EAGAIN) { - //if resource was busy, can try again, otherwise give up - stop(); - res = 0; - retry = false; - } - } else { - //completed successfully - retry = false; - } - } + int res = send(fd(), (void*)buf, size, MSG_DONTWAIT); + if(res < 0) { + log_e("%d", errno); + stop(); + res = 0; } return res; } int WiFiClient::read(uint8_t *buf, size_t size) { - if (!available() || clientSocketHandle == NULL) { + if(!available()) { return -1; } int res = recv(fd(), buf, size, MSG_DONTWAIT); @@ -257,7 +192,7 @@ int WiFiClient::read(uint8_t *buf, size_t size) int WiFiClient::available() { - if (!_connected || clientSocketHandle == NULL) { + if(!_connected) { return 0; } int count; diff --git a/libraries/WiFi/src/WiFiClient.h b/libraries/WiFi/src/WiFiClient.h index 2e2d925710a..a9f14016d02 100644 --- a/libraries/WiFi/src/WiFiClient.h +++ b/libraries/WiFi/src/WiFiClient.h @@ -20,30 +20,21 @@ #ifndef _WIFICLIENT_H_ #define _WIFICLIENT_H_ + #include "Arduino.h" #include "Client.h" +#include class WiFiClientSocketHandle { private: int sockfd; - int refCount; public: - WiFiClientSocketHandle(int fd):sockfd(fd), refCount(1) - { - } - ; - ~WiFiClientSocketHandle(); - - int ref() + WiFiClientSocketHandle(int fd):sockfd(fd) { - return ++refCount; } - int unref() - { - return --refCount; - } + ~WiFiClientSocketHandle(); int fd() { @@ -54,14 +45,13 @@ class WiFiClientSocketHandle { class WiFiClient : public Client { protected: - WiFiClientSocketHandle *clientSocketHandle; + std::shared_ptr clientSocketHandle; bool _connected; public: WiFiClient *next; WiFiClient(); WiFiClient(int fd); - WiFiClient(const WiFiClient &other); ~WiFiClient(); int connect(IPAddress ip, uint16_t port); int connect(const char *host, uint16_t port); From 12c2c1dacc2e276718083bd05f0c5211fa8965cf Mon Sep 17 00:00:00 2001 From: David Schroeder Date: Tue, 28 Feb 2017 22:00:27 -0500 Subject: [PATCH 3/3] Incorporate comments from review Move WiFiClientSocketHandle and fd() into WiFiClient.cpp --- libraries/WiFi/src/WiFiClient.cpp | 32 +++++++++++++++++++++++++++---- libraries/WiFi/src/WiFiClient.h | 26 ++----------------------- 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/libraries/WiFi/src/WiFiClient.cpp b/libraries/WiFi/src/WiFiClient.cpp index 24d81d9a92d..20041bdcdd1 100644 --- a/libraries/WiFi/src/WiFiClient.cpp +++ b/libraries/WiFi/src/WiFiClient.cpp @@ -29,11 +29,25 @@ #undef write #undef read +class WiFiClientSocketHandle { +private: + int sockfd; -WiFiClientSocketHandle::~WiFiClientSocketHandle() -{ - close(sockfd); -} +public: + WiFiClientSocketHandle(int fd):sockfd(fd) + { + } + + ~WiFiClientSocketHandle() + { + close(sockfd); + } + + int fd() + { + return sockfd; + } +}; WiFiClient::WiFiClient():_connected(false),next(NULL) { @@ -276,3 +290,13 @@ bool WiFiClient::operator==(const WiFiClient& rhs) { return clientSocketHandle == rhs.clientSocketHandle && remotePort() == rhs.remotePort() && remoteIP() == rhs.remoteIP(); } + +int WiFiClient::fd() const +{ + if (clientSocketHandle == NULL) { + return -1; + } else { + return clientSocketHandle->fd(); + } +} + diff --git a/libraries/WiFi/src/WiFiClient.h b/libraries/WiFi/src/WiFiClient.h index a9f14016d02..02df4319c09 100644 --- a/libraries/WiFi/src/WiFiClient.h +++ b/libraries/WiFi/src/WiFiClient.h @@ -25,22 +25,7 @@ #include "Client.h" #include -class WiFiClientSocketHandle { -private: - int sockfd; - -public: - WiFiClientSocketHandle(int fd):sockfd(fd) - { - } - - ~WiFiClientSocketHandle(); - - int fd() - { - return sockfd; - } -}; +class WiFiClientSocketHandle; class WiFiClient : public Client { @@ -87,14 +72,7 @@ class WiFiClient : public Client return !this->operator==(rhs); }; - int fd() const - { - if (clientSocketHandle == NULL) { - return -1; - } else { - return clientSocketHandle->fd(); - } - } + int fd() const; int setSocketOption(int option, char* value, size_t len); int setOption(int option, int *value);