Socket closing improvements and tests adjustments #8499

michalpasztamobica · 2018-10-23T08:46:06Z

Description

These changes are an improvement for the hard-fault issue when closing a socket. They are based on the idea described in this and this by @sentinelt.
The API remains unchanged. As per API description: "Referencing a returned pointer after a close() call is not allowed and leads to undefined behaviour." - See Socket.h.
I checked that all Socket-related unittests work fine and do not hard-fault.
In a separate commit there is also a change to icetea tests, to make them behave according to documentation. I also added a new icetea test for TcpSocket::accept() method.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

SeppoTakalo · 2018-10-23T17:38:31Z

TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py

+                       purpose = "Test that TCPSocket::bind(), TCPSocket::listen() and TCPSocket::accept() works",
+                       status = "released",
+                       component= ["mbed-os", "netsocket"],
+                       author = "Juha Ylinen <[email protected]>",


Don't copy&paste the author line.. You can remove the whole line. It is not used anywhere.

SeppoTakalo · 2018-10-23T17:38:47Z

TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py

+                           "duts": {
+                               '*': { #requirements for all nodes
+                                    "count":2,
+                                    "type": "hardware"


This is still missing the application field.

TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py

SeppoTakalo · 2018-10-23T17:41:39Z

features/netsocket/InternetSocket.cpp

@@ -56,17 +61,15 @@ nsapi_error_t InternetSocket::close()
    _lock.lock();

    nsapi_error_t ret = NSAPI_ERROR_OK;
-    if (!_stack)  {
+    if (!_socket)  {
        return ret;


It should return NSAPI_ERROR_NO_SOCKET

features/netsocket/TCPSocket.cpp

KariHaapalehto · 2018-10-24T05:55:36Z

TEST_APPS/testcases/netsocket/TCPSERVER_ACCEPT.py

-        self.command("dut2", "socket " + str(self.client_socket_id) + " delete")
+        self.command("dut1", "socket " + str(server_socket_id) + " close")
+        self.command("dut1", "socket " + str(server_base_socket_id) + " close")
+        self.command("dut2", "socket " + str(self.client_socket_id) + " close")


Could you please move these close-commands to teardown section.
If the test case raise error, then the close-commands are never called

KariHaapalehto · 2018-10-24T05:55:59Z

TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py

+            raise TestStepFail("Received data doesn't match the sent data")
+
+        response = self.command("dut1", "socket " + str(self.server_base_socket_id) + " close")
+        response = self.command("dut1", "socket " + str(socket_id) + " close")


Could you please move these close-commands to teardown section.
If the test case raise error, then the close-commands are never called

KariHaapalehto · 2018-10-24T05:57:17Z

TEST_APPS/testcases/netsocket/TCPSOCKET_ECHOTEST_BURST_SHORT.py

@@ -72,7 +72,7 @@ def case(self):
            if data != sentData:
                raise TestStepFail("Received data doesn't match the sent data")

-        self.command("dut1", "socket " + str(socket_id) + " delete")
+        self.command("dut1", "socket " + str(socket_id) + " close")


Same as above comment

0xc0170 · 2018-10-24T08:56:07Z

@michalpasztamobica When you push an update (more important after review comments), please write a comment here what has changed - to notify reviewers this is ready for another round of reviews. The question is - is it now?

michalpasztamobica · 2018-10-24T09:16:20Z

@SeppoTakalo , the code is ready for review if you agree with my responses to the questions you raised. What I changed:

updated the cmd_socket.cpp, to handle the TCPSocket properly
applied all your and Kari's remarks apart from the two I am discussing above.
adjusted the unit test to match the correct return value,
I also reverted the initial idea of calling close() everywhere instead of delete. It was wrong - for the sockets created by the test using new, we should still call delete - the destructor should call close() for us. If we do not call delete, but only call close(), then the sockets might not be deleted (unless Python's garbage collector takes care of that? - I don't know). So in the end - we should only call close() 1) if we want to reopen the socket later or 2) if it was returned by TCPSocket::accept().

SeppoTakalo

Small changes still required.

Also, make sure your editor is set to use 4 spaces as a indentation. Not tabs.
Some intends seems off

SeppoTakalo · 2018-10-24T10:43:19Z

TEST_APPS/device/socket_app/cmd_socket.cpp

-            return handle_nsapi_error("TCPServer::listen()", static_cast<TCPServer &>(info->socket()).listen(backlog));
+        	if (info->type() == SInfo::TCP_SERVER) {
+        		return handle_nsapi_error("TCPServer::listen()", static_cast<TCPServer &>(info->socket()).listen(backlog));
+        	} else if (info->type() == SInfo::TCP_CLIENT) {


Why type has to be TCP_CLIENT or TCP_SERVER
the Socket::listen() is in the abstract API., so we could call

if (cmd_parameter_int(argc, argv, "listen", &backlog)) { return handle_nsapi_error("Socket::listen()", info->socket().listen(backlog));

SeppoTakalo · 2018-10-24T10:44:43Z

TEST_APPS/device/socket_app/cmd_socket.cpp

-                       new_info->id(), addr.get_ip_address(), addr.get_port());
-        }
-        return handle_nsapi_error("TCPServer::accept()", ret);
+    	nsapi_error_t ret;


Wrong intend here

SeppoTakalo · 2018-10-24T10:47:27Z

TEST_APPS/device/socket_app/cmd_socket.cpp

-        return handle_nsapi_error("TCPServer::accept()", ret);
+    	nsapi_error_t ret;
+		if (info->type() != SInfo::TCP_SERVER) {
+			TCPSocket *new_sock = static_cast<TCPSocket &>(info->socket()).accept(&ret);


No, not TCPSocket, we should be able to use any socket here.
So Socket *new_sock = info->socket().accept(&ret);

TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py

SeppoTakalo · 2018-10-24T10:57:35Z

TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py

+        interface = interfaceUp(self, ["dut2"])
+        self.client_ip = interface["dut2"]["ip"]
+        #response = self.command("dut1", "ifup")
+        #response = self.command("dut2", "ifup")


Remove these

features/netsocket/TCPSocket.cpp

SeppoTakalo · 2018-10-24T11:01:54Z

And.. those cmd_socket.cpp changes.. check that equivalent are going to be done to Cliapp as well. I might have missed those.

The TCPServer accept should use the old API. But the new accept() should not need typecasting.

michalpasztamobica · 2018-10-25T06:33:21Z

@SeppoTakalo , please review
I applied the changes you mentioned, but the cliapp repo source code for the cmd_socket.cpp has diverged quite a lot from what lives in mbed-os repo. I only picked the minimal changes to get the code compile and run so we can this get through with this pull request.
All icetea tests available in mbed-os/TEST_APPS are passing fine.

0xc0170 · 2018-10-25T11:35:26Z

@KariHaapalehto review the update

0xc0170 · 2018-10-25T11:42:47Z

/morph build

mbed-ci · 2018-10-25T13:15:56Z

Build : SUCCESS

Build number : 3464
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8499/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-25T23:53:10Z

Test : FAILURE

Build number : 3255
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8499/3255

mbed-ci · 2018-10-26T02:43:44Z

Exporter Build : SUCCESS

Build number : 3087
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8499/

michalpasztamobica · 2018-10-26T05:55:28Z

Test failures revealed that I did not adjust them to the changes we made.
Now when close() is called, but no socket is available, an NSAPI_ERROR_NO_SOCKET is returned (NSAPI_ERROR_OK was returned before the changes)
I need to adjust all the tests which call close() without a corresponding open() to expect an error code.

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.

michalpasztamobica · 2018-10-26T09:06:26Z

I just pushed a fix agreed with @SeppoTakalo .
The problem was that we removed the stack invalidation step from close(). Actually we do want to invalidate _stack pointer when closing the socket, because a new open() call will provide a new pointer anyway.
I locally checked that all tests which failed are now passing.

0xc0170 · 2018-10-26T09:39:25Z

/morph build

mbed-ci · 2018-10-26T10:41:03Z

Build : SUCCESS

Build number : 3476
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8499/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-26T15:08:36Z

Exporter Build : SUCCESS

Build number : 3096
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8499/

mbed-ci · 2018-10-26T17:42:02Z

Test : SUCCESS

Build number : 3265
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8499/3265

cmonr requested a review from SeppoTakalo October 23, 2018 17:06

cmonr added the needs: review label Oct 23, 2018

cmonr requested review from a team and removed request for SeppoTakalo October 23, 2018 17:06

SeppoTakalo suggested changes Oct 23, 2018

View reviewed changes

KariHaapalehto suggested changes Oct 24, 2018

View reviewed changes

michalpasztamobica force-pushed the master branch 3 times, most recently from 200f975 to 45a7bb3 Compare October 24, 2018 08:13

SeppoTakalo suggested changes Oct 24, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Oct 24, 2018

michalpasztamobica force-pushed the master branch from 45a7bb3 to 1226b97 Compare October 24, 2018 12:17

0xc0170 mentioned this pull request Oct 24, 2018

Fix hard-fault when socket created using accept() is deleted #8337

Closed

michalpasztamobica force-pushed the master branch 3 times, most recently from 5f23045 to 7c16ea3 Compare October 25, 2018 06:28

michalpasztamobica force-pushed the master branch from 7c16ea3 to 2c11a07 Compare October 25, 2018 06:38

SeppoTakalo approved these changes Oct 25, 2018

View reviewed changes

0xc0170 approved these changes Oct 25, 2018

View reviewed changes

SeppoTakalo approved these changes Oct 25, 2018

View reviewed changes

0xc0170 added needs: review and removed needs: work labels Oct 25, 2018

KariHaapalehto approved these changes Oct 25, 2018

View reviewed changes

0xc0170 added the needs: CI label Oct 25, 2018

0xc0170 removed the needs: review label Oct 25, 2018

cmonr added the release-version: 5.10.3 label Oct 25, 2018

cmonr approved these changes Oct 25, 2018

View reviewed changes

michalpasztamobica force-pushed the master branch from 2c11a07 to 0da0f16 Compare October 26, 2018 09:03

SeppoTakalo approved these changes Oct 26, 2018

View reviewed changes

cmonr added ready for merge and removed needs: CI labels Oct 26, 2018

cmonr merged commit 9403a2f into ARMmbed:master Oct 29, 2018

cmonr removed the ready for merge label Oct 29, 2018

Socket closing improvements and tests adjustments #8499

Socket closing improvements and tests adjustments #8499

Uh oh!

Conversation

michalpasztamobica commented Oct 23, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Oct 24, 2018

Uh oh!

michalpasztamobica commented Oct 24, 2018

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SeppoTakalo commented Oct 24, 2018

Uh oh!

michalpasztamobica commented Oct 25, 2018

Uh oh!

0xc0170 commented Oct 25, 2018

Uh oh!

0xc0170 commented Oct 25, 2018

Uh oh!

mbed-ci commented Oct 25, 2018

Triggering tests

Uh oh!

mbed-ci commented Oct 25, 2018

Uh oh!

mbed-ci commented Oct 26, 2018

Uh oh!

michalpasztamobica commented Oct 26, 2018

Uh oh!

michalpasztamobica commented Oct 26, 2018

Uh oh!

0xc0170 commented Oct 26, 2018

Uh oh!

mbed-ci commented Oct 26, 2018

Triggering tests

Uh oh!

mbed-ci commented Oct 26, 2018

Uh oh!

mbed-ci commented Oct 26, 2018

Uh oh!

Uh oh!