[riot-notifications] [RIOT-OS/RIOT] [RFC] gnrc_tcp_recv(): fix connection closed by remote host not handled (#10899)

Thomas Stilwell notifications at github.com
Tue Jan 29 23:26:17 CET 2019

The RIOT community cares a lot about code quality.
Therefore, before describing what your contribution is about, we would like
you to make sure that your modifications are compliant with the RIOT
coding conventions, see https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions.

### Contribution description
I'm not certain this is right, but it fixed the problem I was having and it seems like it's right. Without this PR, I don't see how application code calling `gnrc_tcp_recv()` is notified of the remote host closing the connection. Documentation doesn't say what it should return when the remote end has closed. Currently it just continues blocking until timeout (or else it returns -EAGAIN), so we aren't able to tell that the remote host has closed the connection. This seems wrong.

The way `recv()` works on linux is that it returns 0 if the remote end has closed. The application usually responds to this by closing the local end if it has no more data to send. It seems to me that `gnrc_tcp_recv()` should return 0 so that the application can then call `gnrc_tcp_close()`.

Otherwise what happens is that the connection gets stuck half open because the application never learns that it should close it. That is, `tcb->state` gets stuck in `FSM_STATE_CLOSE_WAIT` and new connections from a client are rejected until the riot node is reset.

Put here the description of your contribution:
- describe which part(s) of RIOT is (are) involved
- if it's a bug fix, describe the bug that it solves and how it is solved
- you can also give more information to reviewers about how to test your changes

### Testing procedure
We don't have anything in tests or examples that uses `gnrc_tcp_recv()` in a way that can test what happens when the remote host closes the connection. What I'm testing with is a [telnet server](/benemorius/RIOT/tree/telnet-shell/examples/telnet_shell) that I implemented against gnrc_tcp. I can connect with a telnet client and then disconnect, but without this PR subsequent connection attempts are refused. With this PR, the connection is closed properly and subsequent connections are successful.

Maybe the telnet server will compile for many platforms but unfortunately I don't have it alone in a clean branch right now and that branch has some hacks in core and sys, so really it's a very non-ideal way to be testing this.

Normally I'd want to stop and write a stripped down test for this, but I don't have a lot of time and this seems like something we should try to address in time for the 2019.01 release unless I'm just mistaken about how it's supposed to work. Anyway I wanted to see what anyone else thought about it since I don't know when I'll have time to continue with it.

Details steps to test your contribution:
- which test/example to compile for which board and is there a 'test' command
- how to know that it was not working/available in master
- the expected success test output

<!--### Issues/PRs references

Examples: Fixes #1234. See also #5678. Depends on PR #9876.

Please use keywords (e.g., fixes, resolve) with the links to the issues you
resolved, this way they will be automatically closed when your pull request
is merged. See https://help.github.com/articles/closing-issues-using-keywords/.

You can view, comment on, or merge this pull request online at:


-- Commit Summary --

  * gnrc_tcp_recv(): fix typo in debug output
  * gnrc_tcp_recv(): return 0 if connection was closed by remote host

-- File Changes --

    M sys/net/gnrc/transport_layer/tcp/gnrc_tcp.c (12)

-- Patch Links --


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190129/d566e358/attachment.html>

More information about the notifications mailing list