TCPSocket accept refactored to close cleanly and icetea test added

Private constructor called in TCPSocket accept, when creating a new Socket.
Close() method calls moved "up" to InternetSocket.
InternetSocket::close() returns proper error code when no socket available.
Add TcpSocket::accept icetea tests.
Deleting sockets moved to teardown.

TEST_APPS/device/socket_app/cmd_socket.cpp

@@ -95,7 +95,8 @@ public:
     enum SocketType {
         TCP_CLIENT,
         TCP_SERVER,
-        UDP
+        UDP,
+        OTHER
     };
     SInfo(TCPSocket *sock):
         _id(id_count++),
@@ -151,6 +152,24 @@ public:
     {
         assert(sock);
     }
+    SInfo(Socket* sock, bool delete_on_exit=true):
+        _id(id_count++),
+        _sock(sock),
+        _type(SInfo::OTHER),
+        _blocking(true),
+        _dataLen(0),
+        _maxRecvLen(0),
+        _repeatBufferFill(1),
+        _receivedTotal(0),
+        _receiverThread(NULL),
+        _receiveBuffer(NULL),
+        _senderThreadId(NULL),
+        _receiverThreadId(NULL),
+        _packetSizes(NULL),
+        _check_pattern(false)
+    {
+        MBED_ASSERT(sock);
+    }
     ~SInfo()
     {
         this->_sock->sigio(Callback<void()>());
@@ -1109,40 +1128,48 @@ static int cmd_socket(int argc, char *argv[])
     }
 
     /*
-     * Commands for TCPServer
+     * Commands for TCPServer and TCPSocket
      * listen, accept
      */
-    if ((COMMAND_IS("listen") || COMMAND_IS("accept")) && info->type() != SInfo::TCP_SERVER) {
-        cmd_printf("Not TCPServer\r\n");
-        return CMDLINE_RETCODE_FAIL;
-    }
     if (COMMAND_IS("listen")) {
         int32_t backlog;
         if (cmd_parameter_int(argc, argv, "listen", &backlog)) {
-            return handle_nsapi_error("TCPServer::listen()", static_cast<TCPServer &>(info->socket()).listen(backlog));
+            return handle_nsapi_error("Socket::listen()", info->socket().listen(backlog));
         } else {
-            return handle_nsapi_error("TCPServer::listen()", static_cast<TCPServer &>(info->socket()).listen());
+            return handle_nsapi_error("Socket::listen()", info->socket().listen());
         }
 
     } else if (COMMAND_IS("accept")) {
-        SocketAddress addr;
+        nsapi_error_t ret;
-        int32_t id;
+        if (info->type() != SInfo::TCP_SERVER) {
-        if (!cmd_parameter_int(argc, argv, "accept", &id)) {
+            Socket *new_sock = info->socket().accept(&ret);
-            cmd_printf("Need new socket id\r\n");
+            if (ret == NSAPI_ERROR_OK) {
-            return CMDLINE_RETCODE_INVALID_PARAMETERS;
+                SInfo *new_info = new SInfo(new_sock);
+                m_sockets.push_back(new_info);
+                cmd_printf("Socket::accept() new socket sid: %d\r\n", new_info->id());
+            }
+            return handle_nsapi_error("Socket::accept()", ret);
+        } else { // Old TCPServer API
+            int32_t id;
+            SocketAddress addr;
+
+            if (!cmd_parameter_int(argc, argv, "accept", &id)) {
+                cmd_printf("Need new socket id\r\n");
+                return CMDLINE_RETCODE_INVALID_PARAMETERS;
+            }
+            SInfo *new_info = get_sinfo(id);
+            if (!new_info) {
+                cmd_printf("Invalid socket id\r\n");
+                return CMDLINE_RETCODE_FAIL;
+            }
+            TCPSocket *new_sock = static_cast<TCPSocket*>(&new_info->socket());
+            nsapi_error_t ret = static_cast<TCPServer&>(info->socket()).accept(new_sock, &addr);
+            if (ret == NSAPI_ERROR_OK) {
+                cmd_printf("TCPServer::accept() new socket sid: %d connection from %s port %d\r\n",
+                        new_info->id(), addr.get_ip_address(), addr.get_port());
+            }
+            return handle_nsapi_error("TCPServer::accept()", ret);
         }
-        SInfo *new_info = get_sinfo(id);
-        if (!new_info) {
-            cmd_printf("Invalid socket id\r\n");
-            return CMDLINE_RETCODE_FAIL;
-        }
-        TCPSocket *new_sock = static_cast<TCPSocket *>(&new_info->socket());
-        nsapi_error_t ret = static_cast<TCPServer &>(info->socket()).accept(new_sock, &addr);
-        if (ret == NSAPI_ERROR_OK) {
-            cmd_printf("TCPServer::accept() new socket sid: %d connection from %s port %d\r\n",
-                       new_info->id(), addr.get_ip_address(), addr.get_port());
-        }
-        return handle_nsapi_error("TCPServer::accept()", ret);
     }
     return CMDLINE_RETCODE_INVALID_PARAMETERS;
 }

TEST_APPS/testcases/netsocket/SOCKET_BIND_PORT.py

@@ -43,15 +43,15 @@ class MultipleTestcase(Bench):
 
     def socket_bind_port(self, socket_type):
         response = self.command("dut1", "socket new " + socket_type)
-        socket_id = int(response.parsed['socket_id'])
+        self.socket_id = int(response.parsed['socket_id'])
 
-        self.command("dut1", "socket " + str(socket_id) + " open")
+        self.command("dut1", "socket " + str(self.socket_id) + " open")
 
-        self.command("dut1", "socket " + str(socket_id) + " bind port 1024")
+        self.command("dut1", "socket " + str(self.socket_id) + " bind port 1024")
-
-        self.command("dut1", "socket " + str(socket_id) + " delete")
 
     def teardown(self):
+        self.command("dut1", "socket " + str(self.socket_id) + " delete")
+        
         self.command("dut1", "ifdown")
 
 

TEST_APPS/testcases/netsocket/TCPSERVER_ACCEPT.py

@@ -62,15 +62,15 @@ class Testcase(Bench):
         self.used_port = 2000
 
         response = self.command("dut1", "socket new TCPServer")
-        server_base_socket_id = int(response.parsed['socket_id'])
+        self.server_base_socket_id = int(response.parsed['socket_id'])
 
-        self.command("dut1", "socket " + str(server_base_socket_id) + " open")
+        self.command("dut1", "socket " + str(self.server_base_socket_id) + " open")
-        self.command("dut1", "socket " + str(server_base_socket_id) + " bind port " + str(self.used_port))
+        self.command("dut1", "socket " + str(self.server_base_socket_id) + " bind port " + str(self.used_port))
-        self.command("dut1", "socket " + str(server_base_socket_id) + " listen")
+        self.command("dut1", "socket " + str(self.server_base_socket_id) + " listen")
 
         response = self.command("dut1", "socket new TCPSocket")
-        server_socket_id = int(response.parsed['socket_id'])
+        self.server_socket_id = int(response.parsed['socket_id'])
-        self.command("dut1", "socket " + str(server_socket_id) + " open")
+        self.command("dut1", "socket " + str(self.server_socket_id) + " open")
 
         response = self.command("dut2", "socket new TCPSocket")
         zero = response.timedelta
@@ -81,7 +81,7 @@ class Testcase(Bench):
         t.start()
 
         wait = 5
-        response = self.command("dut1", "socket " + str(server_base_socket_id) + " accept " + str(server_socket_id))
+        response = self.command("dut1", "socket " + str(self.server_base_socket_id) + " accept " + str(self.server_socket_id))
         response.verify_response_duration(expected=wait, zero=zero, threshold_percent=10, break_in_fail=True)
         socket_id = int(response.parsed['socket_id'])
 
@@ -94,10 +94,10 @@ class Testcase(Bench):
         if data != "hello":
             raise TestStepFail("Received data doesn't match the sent data")
 
-        self.command("dut1", "socket " + str(server_socket_id) + " delete")
-        self.command("dut1", "socket " + str(server_base_socket_id) + " delete")
-        self.command("dut2", "socket " + str(self.client_socket_id) + " delete")
-
     def teardown(self):
+        self.command("dut1", "socket " + str(self.server_socket_id) + " delete")
+        self.command("dut1", "socket " + str(self.server_base_socket_id) + " delete")
+        self.command("dut2", "socket " + str(self.client_socket_id) + " delete")
+        
         interfaceDown(self, ["dut1"])
         interfaceDown(self, ["dut2"])

TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py

@@ -0,0 +1,106 @@
+"""
+Copyright 2018 ARM Limited
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+import threading
+import time
+
+from icetea_lib.bench import Bench
+from interface import interfaceUp, interfaceDown
+#from mbed_clitest.tools import test_case
+#from mbed_clitest.TestStepError import SkippedTestcaseException
+from icetea_lib.TestStepError import TestStepFail
+
+class Testcase(Bench):
+    def __init__(self):
+        Bench.__init__(self,
+                       name="TCPSOCKET_ACCEPT",
+                       title = "TCPSOCKET_ACCEPT",
+                       purpose = "Test that TCPSocket::bind(), TCPSocket::listen() and TCPSocket::accept() works",
+                       status = "released",
+                       component= ["mbed-os", "netsocket"],
+                       type="smoke",
+                       subtype="socket",
+                       requirements={
+                           "duts": {
+                               '*': { #requirements for all nodes
+                                    "count":2,
+                                    "type": "hardware",
+                                    "application": {
+                                       "name": "TEST_APPS-device-socket_app"
+                                    }
+                                },
+                                "1": {"nick": "dut1"},
+                                "2": {"nick": "dut2"}
+                           }
+                       }
+        )
+
+    def setup(self):
+        interface = interfaceUp(self, ["dut1"])
+        self.server_ip = interface["dut1"]["ip"]
+        interface = interfaceUp(self, ["dut2"])
+        self.client_ip = interface["dut2"]["ip"]
+
+    def clientThread(self):
+        self.logger.info("Starting")
+        time.sleep(5) #wait accept from server
+        self.command("dut2", "socket " + str(self.client_socket_id) + " open")
+        self.command("dut2", "socket " + str(self.client_socket_id) + " connect " + str(self.server_ip) + " " + str(self.used_port))
+
+
+    def case(self):
+        self.used_port = 2000
+
+        response = self.command("dut1", "socket new TCPSocket")
+        self.server_base_socket_id = int(response.parsed['socket_id'])
+
+        self.command("dut1", "socket " + str(self.server_base_socket_id) + " open")
+        response = self.command("dut1", "socket " + str(self.server_base_socket_id) + " bind port " + str(self.used_port), report_cmd_fail = False)
+        if response.retcode == -1:
+            if (response.verify_trace("NSAPI_ERROR_UNSUPPORTED", break_in_fail = False)):
+                raise SkippedTestcaseException("UNSUPPORTED")
+            else:
+                TestStepFail("Bind port failed")
+        self.command("dut1", "socket " + str(self.server_base_socket_id) + " listen")
+
+        response = self.command("dut2", "socket new TCPSocket")
+        self.client_socket_id = int(response.parsed['socket_id'])
+
+        #Create a thread which calls client connect()
+        t = threading.Thread(name='clientThread', target=self.clientThread)
+        t.start()
+
+        response = self.command("dut1", "socket " + str(self.server_base_socket_id) + " accept")
+
+        t.join()
+        self.accept_socket_id = int(response.parsed['socket_id'])
+        if response.timedelta < 5.0: # Check that socket accept call blocks
+            raise TestStepFail("Longer response time expected")
+
+        self.command("dut1", "socket " + str(self.accept_socket_id) + " send hello")
+
+        response = self.command("dut2", "socket " + str(self.client_socket_id) + " recv 5")
+        data = response.parsed['data'].replace(":","")
+
+        if data != "hello":
+            raise TestStepFail("Received data doesn't match the sent data")
+
+    def teardown(self):
+        response = self.command("dut2", "socket " + str(self.client_socket_id) + " delete")
+        response = self.command("dut1", "socket " + str(self.server_base_socket_id) + " delete")
+        response = self.command("dut1", "socket " + str(self.accept_socket_id) + " close") 
+        
+        interfaceDown(self, ["dut1"])
+        interfaceDown(self, ["dut2"])

TEST_APPS/testcases/netsocket/TCPSOCKET_ECHOTEST_BURST_SHORT.py

@@ -48,31 +48,30 @@ class Testcase(Bench):
 
     def case(self):
         response = self.command("dut1", "socket new TCPSocket")
-        socket_id = int(response.parsed['socket_id'])
+        self.socket_id = int(response.parsed['socket_id'])
 
-        self.command("dut1", "socket " + str(socket_id) + " open")
+        self.command("dut1", "socket " + str(self.socket_id) + " open")
-        self.command("dut1", "socket " + str(socket_id) + " connect echo.mbedcloudtesting.com 7")
+        self.command("dut1", "socket " + str(self.socket_id) + " connect echo.mbedcloudtesting.com 7")
 
         for i in range(2):
             sentData = ""
             for size in (100, 200, 300, 120, 500):
                 packet = Randomize.random_string(max_len=size, min_len=size, chars=string.ascii_uppercase)
                 sentData += packet
-                response = self.command("dut1", "socket " + str(socket_id) + " send " + str(packet))
+                response = self.command("dut1", "socket " + str(self.socket_id) + " send " + str(packet))
                 response.verify_trace("TCPSocket::send() returned: " + str(size))
 
             received = 0
             data = ""
             totalSize = 1220
             while received < totalSize:
-                response = self.command("dut1", "socket " + str(socket_id) + " recv " + str(totalSize))
+                response = self.command("dut1", "socket " + str(self.socket_id) + " recv " + str(totalSize))
                 data += response.parsed['data'].replace(":", "")
                 received += int(response.parsed['received_bytes'])
 
             if data != sentData:
                 raise TestStepFail("Received data doesn't match the sent data")
 
-        self.command("dut1", "socket " + str(socket_id) + " delete")
-
     def teardown(self):
+        self.command("dut1", "socket " + str(self.socket_id) + " delete")
         self.command("dut1", "ifdown")

UNITTESTS/features/netsocket/InternetSocket/test_InternetSocket.cpp

@@ -151,7 +151,7 @@ TEST_F(TestInternetSocket, close)
 TEST_F(TestInternetSocket, close_no_open)
 {
     stack.return_value = NSAPI_ERROR_OK;
-    EXPECT_EQ(socket->close(), NSAPI_ERROR_OK);
+    EXPECT_EQ(socket->close(), NSAPI_ERROR_NO_SOCKET);
 }
 
 TEST_F(TestInternetSocket, close_during_read)

features/netsocket/InternetSocket.cpp

@@ -26,6 +26,11 @@ InternetSocket::InternetSocket()
 {
 }
 
+InternetSocket::~InternetSocket()
+{
+    close();
+}
+
 nsapi_error_t InternetSocket::open(NetworkStack *stack)
 {
     _lock.lock();
@@ -56,17 +61,16 @@ nsapi_error_t InternetSocket::close()
     _lock.lock();
 
     nsapi_error_t ret = NSAPI_ERROR_OK;
-    if (!_stack)  {
+    if (!_socket)  {
-        return ret;
+        return NSAPI_ERROR_NO_SOCKET;
     }
 
-    if (_socket) {
+    // Just in case - tell the stack not to callback any more, then remove this socket.
-        _stack->socket_attach(_socket, 0, 0);
+    _stack->socket_attach(_socket, 0, 0);
-        nsapi_socket_t socket = _socket;
+    nsapi_socket_t socket = _socket;
-        _socket = 0;
+    _socket = 0;
-        ret = _stack->socket_close(socket);
+    ret = _stack->socket_close(socket);
-    }
+    _stack = 0; // Invalidate the stack pointer - otherwise open() fails.
-    _stack = 0;
 
     // Wakeup anything in a blocking operation
     // on this socket
@@ -81,7 +85,7 @@ nsapi_error_t InternetSocket::close()
 
     _lock.unlock();
 
-    // When allocated by accept() call, will self desctruct on close();
+    // When allocated by accept() call, will destroy itself on close();
     if (_factory_allocated) {
         delete this;
     }

features/netsocket/InternetSocket.h

@@ -36,7 +36,7 @@ public:
      *
      *  Closes socket if the socket is still open
      */
-    virtual ~InternetSocket() {}
+    virtual ~InternetSocket();
 
     /** Opens a socket
      *

features/netsocket/TCPServer.cpp

@@ -24,7 +24,6 @@ TCPServer::TCPServer()
 
 TCPServer::~TCPServer()
 {
-    close();
 }
 
 nsapi_error_t TCPServer::accept(TCPSocket *connection, SocketAddress *address)

features/netsocket/TCPSocket.cpp

@@ -22,9 +22,19 @@ TCPSocket::TCPSocket()
 {
 }
 
+TCPSocket::TCPSocket(TCPSocket* parent, nsapi_socket_t socket, SocketAddress address)
+{
+    _socket = socket,
+    _stack = parent->_stack;
+    _factory_allocated = true;
+    _remote_peer = address;
+
+    _event = mbed::Callback<void()>(this, &TCPSocket::event);
+    _stack->socket_attach(socket, &mbed::Callback<void()>::thunk, &_event);
+}
+
 TCPSocket::~TCPSocket()
 {
-    close();
 }
 
 nsapi_protocol_t TCPSocket::get_proto()
@@ -265,16 +275,7 @@ TCPSocket *TCPSocket::accept(nsapi_error_t *error)
         ret = _stack->socket_accept(_socket, &socket, &address);
 
         if (0 == ret) {
-            connection = new TCPSocket();
+            connection = new TCPSocket(this, socket, address);
-            connection->_lock.lock();
-            connection->_factory_allocated = true; // Destroy automatically on close()
-            connection->_remote_peer = address;
-            connection->_stack = _stack;
-            connection->_socket = socket;
-            connection->_event = mbed::Callback<void()>(connection, &TCPSocket::event);
-            _stack->socket_attach(socket, &mbed::Callback<void()>::thunk, &connection->_event);
-
-            connection->_lock.unlock();
             break;
         } else if ((_timeout == 0) || (ret != NSAPI_ERROR_WOULD_BLOCK)) {
             break;

features/netsocket/TCPSocket.h

@@ -182,6 +182,13 @@ public:
 protected:
     friend class TCPServer;
     virtual nsapi_protocol_t get_proto();
+
+private:
+    /** Create a socket out of a given socket
+     *
+     *  To be used within accept() function. Close() will clean this up.
+     */
+    TCPSocket(TCPSocket *parent, nsapi_socket_t socket, SocketAddress address);
 };
 
 

features/netsocket/UDPSocket.cpp

@@ -24,7 +24,6 @@ UDPSocket::UDPSocket()
 
 UDPSocket::~UDPSocket()
 {
-    close();
 }
 
 nsapi_protocol_t UDPSocket::get_proto()