New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Socket closing improvements and tests adjustments #8499

Merged
merged 1 commit into from Oct 29, 2018

Conversation

Projects
None yet
6 participants
@michalpasztamobica
Contributor

michalpasztamobica commented Oct 23, 2018

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

@cmonr cmonr requested a review from SeppoTakalo Oct 23, 2018

@cmonr cmonr added the needs: review label Oct 23, 2018

@cmonr cmonr requested review from ARMmbed/mbed-os-ipcore and removed request for SeppoTakalo Oct 23, 2018

purpose = "Test that TCPSocket::bind(), TCPSocket::listen() and TCPSocket::accept() works",
status = "released",
component= ["mbed-os", "netsocket"],
author = "Juha Ylinen <juha.ylinen@arm.com>",

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 23, 2018

Contributor

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

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

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 23, 2018

Contributor

This is still missing the application field.

Show resolved Hide resolved TEST_APPS/testcases/netsocket/TCPSOCKET_ACCEPT.py
@@ -56,17 +61,15 @@ nsapi_error_t InternetSocket::close()
_lock.lock();
nsapi_error_t ret = NSAPI_ERROR_OK;
if (!_stack) {
if (!_socket) {
return ret;

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 23, 2018

Contributor

It should return NSAPI_ERROR_NO_SOCKET

Show resolved Hide resolved features/netsocket/TCPSocket.cpp
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")

This comment has been minimized.

@KariHaapalehto

KariHaapalehto Oct 24, 2018

Contributor

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

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")

This comment has been minimized.

@KariHaapalehto

KariHaapalehto Oct 24, 2018

Contributor

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

@@ -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")

This comment has been minimized.

@KariHaapalehto

KariHaapalehto Oct 24, 2018

Contributor

Same as above comment

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:master branch 3 times, most recently from 200f975 to 45a7bb3 Oct 24, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 24, 2018

@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

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 24, 2018

@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

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) {

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 24, 2018

Contributor

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));
new_info->id(), addr.get_ip_address(), addr.get_port());
}
return handle_nsapi_error("TCPServer::accept()", ret);
nsapi_error_t ret;

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 24, 2018

Contributor

Wrong intend here

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);

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 24, 2018

Contributor

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

Show resolved Hide resolved 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")

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Oct 24, 2018

Contributor

Remove these

Show resolved Hide resolved features/netsocket/TCPSocket.cpp
@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Oct 24, 2018

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.

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

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:master branch from 45a7bb3 to 1226b97 Oct 24, 2018

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:master branch 3 times, most recently from 5f23045 to 7c16ea3 Oct 25, 2018

@michalpasztamobica

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 25, 2018

@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.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:master branch from 7c16ea3 to 2c11a07 Oct 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 25, 2018

@KariHaapalehto review the update

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

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

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 25, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 25, 2018

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

@cmonr

cmonr approved these changes Oct 25, 2018

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@michalpasztamobica

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 26, 2018

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.

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.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:master branch from 2c11a07 to 0da0f16 Oct 26, 2018

@michalpasztamobica

This comment has been minimized.

Contributor

michalpasztamobica commented Oct 26, 2018

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

This comment has been minimized.

Member

0xc0170 commented Oct 26, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 26, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

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

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

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success, null
Details
travis-ci/astyle Passed, 538 files (+2 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9186 cycles (+3 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment