Skip to content
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

TCP server fixes #1286

Merged
merged 9 commits into from May 10, 2017

Conversation

@sergeuz
Copy link
Member

commented Apr 3, 2017

This PR accompanies TCP server fixes in WICED and introduces the following changes to improve stability of the firmware's TCP server implementation:

  • Update server's list of clients on a client destruction (thanks @tlangmo!).
  • TCPClient now closes underlying socket on destruction.
  • Test app for above fixes (including fixes in WICED).

Testing

Flash server application to the device:
 $ cd ~/firmware/modules
 $ make -s all program-dfu PLATFORM=photon TEST=app/tcp_server
 
 Install client dependencies:
 $ cd ~/firmware/user/tests/app/tcp_server/client
 $ npm install
 
 Run client:
 $ ./client

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@technobly technobly added this to the 0.7.0 milestone Apr 4, 2017

@technobly technobly changed the base branch from develop to feature/photon/wiced-3.7.0-7 May 4, 2017

@technobly technobly requested a review from avtolstoy May 4, 2017

@avtolstoy
Copy link
Member

left a comment

Looks good. Tests passed as well.

Running EchoTest...
Connected to network, device address: 10.40.30.63
Started server: server1, port: 1234 (EchoServer)
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Stopped all servers
PASSED

Running MaxConnectionsTest...
Started server: server1, port: 1234 (EchoServer)
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Stopped all servers
PASSED

Running ExtraConnectionTest...
Started server: server1, port: 1234 (EchoServer)
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Stopped all servers
PASSED

Running MaxServersTest...
Started server: server1, port: 1234 (EchoServer)
Started server: server2, port: 1235 (EchoServer)
Started server: server3, port: 1236 (EchoServer)
Started server: server4, port: 1237 (EchoServer)
Started server: server5, port: 1238 (EchoServer)
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1235
Connected to 10.40.30.63:1236
Connected to 10.40.30.63:1237
Connected to 10.40.30.63:1238
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Stopped all servers
PASSED

Running NoServerSocketLeakOnNetworkDisconnectTest...
Started server: server1, port: 1234 (SimpleServer)
Started server: server2, port: 1235 (SimpleServer)
Started server: server3, port: 1236 (SimpleServer)
Started server: server4, port: 1237 (SimpleServer)
Started server: server5, port: 1238 (SimpleServer)
Disconnected from network
Stopped all servers
Connected to network, device address: 10.40.30.63
Started server: server6, port: 1234 (SimpleServer)
Started server: server7, port: 1235 (SimpleServer)
Started server: server8, port: 1236 (SimpleServer)
Started server: server9, port: 1237 (SimpleServer)
Started server: server10, port: 1238 (SimpleServer)
Stopped all servers
PASSED

Running StoppedServerDoesntAffectOtherServersTest...
Started server: server1, port: 1234 (EchoServer)
Started server: server2, port: 1235 (EchoServer)
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Connected to 10.40.30.63:1235
Disconnected from 10.40.30.63
Stopped server: server1
Connected to 10.40.30.63:1235
Disconnected from 10.40.30.63
Stopped all servers
PASSED

Running StoppingServerWithActiveConnectionsTest...
Started server: server1, port: 1234 (SimpleServer)
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Disconnected from 10.40.30.63
Stopped server: server1
PASSED

Running TcpClientClosesOnDestructionTest...
Started server: server1, port: 1234 (TcpClientClosesOnDestructionTestServer)
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Connected to 10.40.30.63:1234
Disconnected from 10.40.30.63
Stopped all servers
PASSED

Running StressTest...
Total connections: 500
Concurrent connection attempts: 5
Allowed failed connections: 6
Connecting...
PASSED

PASSED: All 9 tests passed
@@ -46,7 +44,8 @@ TCPClient::TCPClient() : TCPClient(socket_handle_invalid())
{
}

TCPClient::TCPClient(sock_handle_t sock) : _sock(sock)
TCPClient::TCPClient(sock_handle_t sock) :
d_(new Data(sock))

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 8, 2017

Member

This could better be std::make_shared.

@technobly technobly merged commit 5ddc904 into feature/photon/wiced-3.7.0-7 May 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the fix/tcp_server branch May 10, 2017

avtolstoy added a commit to avtolstoy/firmware that referenced this pull request May 10, 2017
avtolstoy pushed a commit to avtolstoy/firmware that referenced this pull request May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.