Add Open Port Request on Sending Side of IpSocket (#2683)

* remove thread specific behavior from the library level of IpSocket. Moved behavior to the component level by placing thread protection behavior to SocketReadTask, now called SocketComponentHelper. Calls to socket functionality from TcpClient/TcpServer/Udp will go through SocketComponentHelper rather than directly to the library. This also implemented the original desired functionality of reopening a client socket on a send call rather than only in receive

* merge new logger name

* update unit tests for new structuring and update smake to allow unit tests whos name includes more than the module name

* updates and fixes for TcpClient's unit tests

* fix/work around for an interesting issue where the TcpClient receive buffer equality check would fail because the size of the original buffer (owned by the unit test) gets set to 0, sometimes. Maybe 1/10 runs. I changed it to save the size of that buffer before it does the send and use that saved value in the equality check

* fix tcp server and tcp client unit tests (hopefully)

* update UDP uts

* fix startup function change

* fix reference argument and mutex locking to take care of issues uncovered in integration testing

* updating docs

* updates to remove dead code and comments

---------

Co-authored-by: crsmith <[email protected]>

.github/actions/spelling/expect.txt

@@ -201,6 +201,7 @@ CRCs
 crcstat
 CREATEDIRECTORY
 Crosscompiling
+crsmith
 crt
 CRTSCTS
 cryptsoft

Drv/Ip/CMakeLists.txt

@@ -12,7 +12,7 @@ set(SOURCE_FILES
 	"${CMAKE_CURRENT_LIST_DIR}/TcpClientSocket.cpp"
 	"${CMAKE_CURRENT_LIST_DIR}/TcpServerSocket.cpp"
 	"${CMAKE_CURRENT_LIST_DIR}/UdpSocket.cpp"
-	"${CMAKE_CURRENT_LIST_DIR}/SocketReadTask.cpp"
+	"${CMAKE_CURRENT_LIST_DIR}/SocketComponentHelper.cpp"
 )
 
 set(MOD_DEPS

Drv/Ip/IpSocket.cpp

@@ -1,6 +1,6 @@
 // ======================================================================
 // \title  IpSocket.cpp
-// \author mstarch
+// \author mstarch, crsmith
 // \brief  cpp file for IpSocket core implementation classes
 //
 // \copyright
@@ -46,20 +46,18 @@
 
 namespace Drv {
 
-IpSocket::IpSocket() : m_fd(-1), m_timeoutSeconds(0), m_timeoutMicroseconds(0), m_port(0), m_open(false), m_started(false) {
+IpSocket::IpSocket() : m_timeoutSeconds(0), m_timeoutMicroseconds(0), m_port(0) {
     ::memset(m_hostname, 0, sizeof(m_hostname));
 }
 
 SocketIpStatus IpSocket::configure(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) {
     FW_ASSERT(timeout_microseconds < 1000000, static_cast<FwAssertArgType>(timeout_microseconds));
     FW_ASSERT(this->isValidPort(port));
     FW_ASSERT(hostname != nullptr);
-    this->m_lock.lock();
     this->m_timeoutSeconds = timeout_seconds;
     this->m_timeoutMicroseconds = timeout_microseconds;
     this->m_port = port;
     (void) Fw::StringUtils::string_copy(this->m_hostname, hostname, static_cast<FwSizeType>(SOCKET_MAX_HOSTNAME_SIZE));
-    this->m_lock.unlock();
     return SOCK_SUCCESS;
 }
 
@@ -105,88 +103,44 @@ SocketIpStatus IpSocket::addressToIp4(const char* address, void* ip4) {
     return SOCK_SUCCESS;
 }
 
-bool IpSocket::isStarted() {
-    bool is_started = false;
-    this->m_lock.lock();
-    is_started = this->m_started;
-    this->m_lock.unLock();
-    return is_started;
+void IpSocket::close(NATIVE_INT_TYPE fd) {
+    (void)::shutdown(fd, SHUT_RDWR);
+    (void)::close(fd);
 }
 
-bool IpSocket::isOpened() {
-    bool is_open = false;
-    this->m_lock.lock();
-    is_open = this->m_open;
-    this->m_lock.unLock();
-    return is_open;
-}
-
-void IpSocket::close() {
-    this->m_lock.lock();
-    if (this->m_fd != -1) {
-        (void)::shutdown(this->m_fd, SHUT_RDWR);
-        (void)::close(this->m_fd);
-        this->m_fd = -1;
-    }
-    this->m_open = false;
-    this->m_lock.unLock();
-}
-
-void IpSocket::shutdown() {
-    this->close();
-    this->m_lock.lock();
-    this->m_started = false;
-    this->m_lock.unLock();
+void IpSocket::shutdown(NATIVE_INT_TYPE fd) {
+    this->close(fd);
 }
 
 SocketIpStatus IpSocket::startup() {
-    this->m_lock.lock();
-    this->m_started = true;
-    this->m_lock.unLock();
+    // no op for non-server components
     return SOCK_SUCCESS;
 }
 
-SocketIpStatus IpSocket::open() {
-    NATIVE_INT_TYPE fd = -1;
+SocketIpStatus IpSocket::open(NATIVE_INT_TYPE& fd) {
     SocketIpStatus status = SOCK_SUCCESS;
-    this->m_lock.lock();
-    FW_ASSERT(m_fd == -1 and not m_open); // Ensure we are not opening an opened socket
-    this->m_lock.unlock();
     // Open a TCP socket for incoming commands, and outgoing data if not using UDP
     status = this->openProtocol(fd);
     if (status != SOCK_SUCCESS) {
-        FW_ASSERT(m_fd == -1); // Ensure we properly kept closed on error
+        FW_ASSERT(fd == -1); // Ensure we properly kept closed on error
         return status;
     }
-    // Lock to update values and "officially open"
-    this->m_lock.lock();
-    this->m_fd = fd;
-    this->m_open = true;
-    this->m_lock.unLock();
     return status;
 }
 
-SocketIpStatus IpSocket::send(const U8* const data, const U32 size) {
+SocketIpStatus IpSocket::send(NATIVE_INT_TYPE fd, const U8* const data, const U32 size) {
     U32 total = 0;
     I32 sent  = 0;
-    this->m_lock.lock();
-    NATIVE_INT_TYPE fd = this->m_fd;
-    this->m_lock.unlock();
-    // Prevent transmission before connection, or after a disconnect
-    if (fd == -1) {
-        return SOCK_DISCONNECTED;
-    }
     // Attempt to send out data and retry as necessary
     for (U32 i = 0; (i < SOCKET_MAX_ITERATIONS) && (total < size); i++) {
         // Send using my specific protocol
-        sent = this->sendProtocol(data + total, size - total);
+        sent = this->sendProtocol(fd, data + total, size - total);
         // Error is EINTR or timeout just try again
         if (((sent == -1) && (errno == EINTR)) || (sent == 0)) {
             continue;
         }
         // Error bad file descriptor is a close along with reset
         else if ((sent == -1) && ((errno == EBADF) || (errno == ECONNRESET))) {
-            this->close();
             return SOCK_DISCONNECTED;
         }
         // Error returned, and it wasn't an interrupt nor a disconnect
@@ -205,27 +159,19 @@ SocketIpStatus IpSocket::send(const U8* const data, const U32 size) {
     return SOCK_SUCCESS;
 }
 
-SocketIpStatus IpSocket::recv(U8* data, U32& req_read) {
+SocketIpStatus IpSocket::recv(NATIVE_INT_TYPE fd, U8* data, U32& req_read) {
     I32 size = 0;
-    // Check for previously disconnected socket
-    this->m_lock.lock();
-    NATIVE_INT_TYPE fd = this->m_fd;
-    this->m_lock.unlock();
-    if (fd == -1) {
-        return SOCK_DISCONNECTED;
-    }
 
     // Try to read until we fail to receive data
     for (U32 i = 0; (i < SOCKET_MAX_ITERATIONS) && (size <= 0); i++) {
         // Attempt to recv out data
-        size = this->recvProtocol(data, req_read);
+        size = this->recvProtocol(fd, data, req_read);
         // Error is EINTR, just try again
         if (size == -1 && ((errno == EINTR) || errno == EAGAIN)) {
             continue;
         }
         // Zero bytes read reset or bad ef means we've disconnected
         else if (size == 0 || ((size == -1) && ((errno == ECONNRESET) || (errno == EBADF)))) {
-            this->close();
             req_read = static_cast<U32>(size);
             return SOCK_DISCONNECTED;
         }

Drv/Ip/IpSocket.hpp

@@ -68,24 +68,6 @@ class IpSocket {
      */
     SocketIpStatus configure(const char* hostname, const U16 port, const U32 send_timeout_seconds,
                              const U32 send_timeout_microseconds);
-    /**
-     * \brief Returns true when the socket is started
-     *
-     * Returns true when the socket is started up sufficiently to be actively listening to clients. Returns false
-     * otherwise. This means `startup()` was called and returned success.
-     */
-    bool isStarted();
-
-    /**
-     * \brief check if IP socket has previously been opened
-     *
-     * Check if this IpSocket has previously been opened. In the case of Udp this will check for outgoing transmissions
-     * and (if configured) incoming transmissions as well. This does not guarantee errors will not occur when using this
-     * socket as the remote component may have disconnected.
-     *
-     * \return true if socket is open, false otherwise
-     */
-    bool isOpened();
 
     /**
      * \brief startup the socket, a no-op on unless this is server
@@ -112,9 +94,10 @@ class IpSocket {
      *
      * Note: delegates to openProtocol for protocol specific implementation
      *
+     * \param fd: file descriptor to open
      * \return status of open
      */
-    SocketIpStatus open();
+    SocketIpStatus open(NATIVE_INT_TYPE& fd);
     /**
      * \brief send data out the IP socket from the given buffer
      *
@@ -126,11 +109,12 @@ class IpSocket {
      *
      * Note: delegates to `sendProtocol` to send the data
      *
+     * \param fd: file descriptor to send to
      * \param data: pointer to data to send
      * \param size: size of data to send
      * \return status of the send, SOCK_DISCONNECTED to reopen, SOCK_SUCCESS on success, something else on error
      */
-    SocketIpStatus send(const U8* const data, const U32 size);
+    SocketIpStatus send(NATIVE_INT_TYPE fd, const U8* const data, const U32 size);
     /**
      * \brief receive data from the IP socket from the given buffer
      *
@@ -142,26 +126,31 @@ class IpSocket {
      *
      * Note: delegates to `recvProtocol` to send the data
      *
+     * \param fd: file descriptor to recv from
      * \param data: pointer to data to fill with received data
      * \param size: maximum size of data buffer to fill
      * \return status of the send, SOCK_DISCONNECTED to reopen, SOCK_SUCCESS on success, something else on error
      */
-    SocketIpStatus recv(U8* const data, U32& size);
+    SocketIpStatus recv(NATIVE_INT_TYPE fd, U8* const data, U32& size);
     /**
      * \brief closes the socket
      *
      * Closes the socket opened by the open call. In this case of the TcpServer, this does NOT close server's listening
      * port (call `shutdown`) but will close the active client connection.
+     * 
+     * \param fd: file descriptor to close
      */
-    void close();
+    void close(NATIVE_INT_TYPE fd);
 
     /**
      * \brief shutdown the socket
      *
      * Closes the socket opened by the open call. In this case of the TcpServer, this does close server's listening
      * port. This will shutdown all clients.
+     * 
+     * \param fd: file descriptor to shutdown
      */
-    virtual void shutdown();
+    virtual void shutdown(NATIVE_INT_TYPE fd);
 
   PROTECTED:
     /**
@@ -198,27 +187,25 @@ class IpSocket {
     virtual SocketIpStatus openProtocol(NATIVE_INT_TYPE& fd) = 0;
     /**
      * \brief Protocol specific implementation of send.  Called directly with retry from send.
+     * \param fd: file descriptor to send to
      * \param data: data to send
      * \param size: size of data to send
      * \return: size of data sent, or -1 on error.
      */
-    virtual I32 sendProtocol(const U8* const data, const U32 size) = 0;
+    virtual I32 sendProtocol(NATIVE_INT_TYPE fd, const U8* const data, const U32 size) = 0;
 
     /**
      * \brief Protocol specific implementation of recv.  Called directly with error handling from recv.
+     * \param fd: file descriptor to recv from
      * \param data: data pointer to fill
      * \param size: size of data buffer
      * \return: size of data received, or -1 on error.
      */
-    virtual I32 recvProtocol( U8* const data, const U32 size) = 0;
+    virtual I32 recvProtocol(NATIVE_INT_TYPE fd, U8* const data, const U32 size) = 0;
 
-    Os::Mutex m_lock;
-    NATIVE_INT_TYPE m_fd;
     U32 m_timeoutSeconds;
     U32 m_timeoutMicroseconds;
     U16 m_port;  //!< IP address port used
-    bool m_open; //!< Have we successfully opened
-    bool m_started; //!< Have we successfully started the socket
     char m_hostname[SOCKET_MAX_HOSTNAME_SIZE];  //!< Hostname to supply
 };
 }  // namespace Drv