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

sock: tcp: fix sock_tcp_read() example #16743

Merged
merged 1 commit into from
Sep 22, 2021

Conversation

benpicco
Copy link
Contributor

Contribution description

sock_tcp_read() will return 0 if the connection was closed, we need to check for this.

Testing procedure

Issues/PRs references

#16494 (comment)

@benpicco benpicco requested a review from maribu as a code owner August 15, 2021 17:59
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Aug 15, 2021
@benpicco benpicco added Area: doc Area: Documentation Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 15, 2021
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 15, 2021
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the example is currently conaistent with the API doc. According to the current API doc, there is no way to detect when a TCP connection is closed. That obviously is a design flaw in need to be fixed. I trust that the implementation already has been fixed, but fixing the was forgotten. Care to add a commit to fiz this? Also, the @retval Doxygen command would make sense for documenting the individual magic return constants.

@benpicco
Copy link
Contributor Author

TBH I'm a bit surprised it does not return -ECONNABORTED when the remote side closes the connection.

@maribu
Copy link
Member

maribu commented Aug 16, 2021

Maybe it is worth to discuss the correct behaviour first.

But note: Both POSIX read() and recv() return 0 at end of file/stream. For that reason, they had to introduce non-error error-codes for nonblocking operation to indicate that no data is available yet. This kind of proves that not using an error code for EOF / closed connection was a bad idea. But still, since the SOCK API was modeled after BSD sockets, I think there will be some confusion with different behaviour.

@brummer-simon
Copy link
Member

brummer-simon commented Aug 16, 2021

TBH I'm a bit surprised it does not return -ECONNABORTED when the remote side closes the connection.

-ECONNABORTED is used as an error condition. Closing a connection normally is not an error. TCP assumes that a connection can be closed from any participating peer at any time regardless of what the peer is currently doing.

Think of recv for example:
-ECONNABORTED is returned if there was no message received from the peer within the last to minutes assuming the connection broke or the peer crashed.

sock_tcp_read() will return 0 if the connection was closed, we
need to check for this.
@benpicco benpicco force-pushed the net/sock/tcp-doc_fix branch from ea236b4 to c3849ed Compare August 16, 2021 10:55
@github-actions github-actions bot removed the Area: doc Area: Documentation label Aug 16, 2021
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 16, 2021
@benpicco benpicco added Area: doc Area: Documentation and removed Area: sys Area: System labels Aug 25, 2021
@benpicco benpicco requested a review from cgundogan September 14, 2021 22:05
@benpicco benpicco added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Sep 17, 2021
@benpicco benpicco requested a review from fjmolinas September 17, 2021 15:34
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and @brummer-simon is fine with the change. ACK.

@benpicco benpicco merged commit b1af25d into RIOT-OS:master Sep 22, 2021
@benpicco benpicco deleted the net/sock/tcp-doc_fix branch September 22, 2021 18:58
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants