Skip to content

gnrc_tcp: return immediatly on gnrc_tcp_recv if a connection is closing #12367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 29, 2019
Merged

gnrc_tcp: return immediatly on gnrc_tcp_recv if a connection is closing #12367

merged 1 commit into from
Oct 29, 2019

Conversation

brummer-simon
Copy link
Member

@brummer-simon brummer-simon commented Oct 3, 2019 •
edited
Loading

This PR fixes the stale Issue mentioned in this PR #10899. I think the original Author abandoned his PR, but the Issue remains till today. To summarize the Issue the behaviour of gnrc_tcp_recv() was not fully in line with the TCP RFC.

Specified Behaviour:
gnrc_tcp_recv() must return immediately if a packet with the FIN Bit set, was received from the peer indicating that want to close a connection. As a response a call to gnrc_tcp_recv() return all queued data up to this point immediately, a given timeout must be ignored because there will be no new data to receive.

Actual Behaviour without this PR:
gnrc_recv() will block even if a packet with the FIN Bit set, was received, if a timeout was given.
Additionally the user of gnrc_tcp has no means to figure out that the peer wants to close the connection.

A more detailed discussion on the Issue can be found in the original PR #10899.

@benpicco benpicco added Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 3, 2019
@miri64
Copy link
Member

miri64 commented Oct 3, 2019

Can you give both your commit and the PR a bit more human readable summary? ;-)

@miri64 miri64 mentioned this pull request Oct 3, 2019
Closed
@brummer-simon brummer-simon changed the title gnrc_tcp-fix_recv_conn_closed gnrc_tcp: return immediatly on gnrc_tcp_recv if a connection is closing Oct 5, 2019
@brummer-simon
Copy link
Member Author

Can you give both your commit and the PR a bit more human readable summary? ;-)

Better now?

@brummer-simon
Copy link
Member Author

@miri64: If this #12369 gets merged before this PR ping me because that will be test script no 6 then.

miri64
miri64 reviewed Oct 5, 2019
View reviewed changes
@brummer-simon brummer-simon mentioned this pull request Oct 6, 2019
Merged
@brummer-simon
Copy link
Member Author

I have changed the test index number to 6 because I think #12369 will be merged before this.

miri64
miri64 requested changes Oct 6, 2019
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Changes seem valid to me, however: the tests should go into a separate commit and there is a change that seems unrelated in here, which also should go into its own commit.

@miri64 miri64 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 Oct 6, 2019
@kaspar030 kaspar030 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 Oct 6, 2019
@miri64
Copy link
Member

miri64 commented Oct 7, 2019 •
edited
Loading

Needs rebase. You may squash (and separate) while doing so.

@brummer-simon brummer-simon force-pushed the gnrc_tcp-fix_recv_conn_closed branch from 4f2823c to dfef54f Compare October 17, 2019 17:56
@brummer-simon
Copy link
Member Author

Rebased to current master. To stabilize the tests I had to increase the timeout of the second script.

@brummer-simon
Copy link
Member Author

@miri64 - Ping due to force push.

miri64
miri64 approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Code looks ok, documentation is provided.

Tested on both native and samr21-xpro. The test runs a bit flaky on samr21-xpro (04 is sometimes failing and sometimes also 05), but that is also the case on master and so far I did not see the newly introduced test suite 6 fail. ACK

@brummer-simon brummer-simon force-pushed the gnrc_tcp-fix_recv_conn_closed branch from dfef54f to 2c961d3 Compare October 22, 2019 15:40
miri64
miri64 requested changes Oct 22, 2019
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Please fix the commit message to something more human-readable ;-)

@brummer-simon brummer-simon force-pushed the gnrc_tcp-fix_recv_conn_closed branch from 2c961d3 to 1eb2969 Compare October 22, 2019 18:17
@brummer-simon
Copy link
Member Author

Please fix the commit message to something more human-readable ;-)

Done

@brummer-simon
Copy link
Member Author

@miri64 - Ping

miri64
miri64 approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK, nothing changed since I last looked at this PR.

@miri64 miri64 merged commit 05d3381 into RIOT-OS:master Oct 29, 2019
@brummer-simon brummer-simon deleted the gnrc_tcp-fix_recv_conn_closed branch October 29, 2019 09:40
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
@fjmolinas
Copy link
Contributor

@brummer-simon did you run the tests on non-native boards as well? I'm having issues making them pass on other boards than native, should that be expected? Might I be missing some configuration?

@brummer-simon
Copy link
Member Author

brummer-simon commented Jan 21, 2020 •
edited
Loading

@brummer-simon did you run the tests on non-native boards as well? I'm having issues making them pass on other boards than native, should that be expected? Might I be missing some configuration?

Hi @fjmolinas,

The tests run on boards as well via ethos. However some boards are faster than others there might be cases where the Timeout specified in the test scripts is to short. Would you do me a favor and increase the timeout for the failing tests? If this works, would you create a PR?

Cheers Simon

@fjmolinas
Copy link
Contributor

The tests run on boards as well via ethos. However some boards are fast than others there might be cases where the Timeout specified in the test scripts is to short. Would you do me a favor and increase the timeout for the failing tests? If this works, would you create a PR?

Thanks, I'm not sure what happened, I had a lot of issue making it work, but eventually it did with the default timeout. Thanks!

@brummer-simon brummer-simon mentioned this pull request Aug 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.
5 participants
@brummer-simon @miri64 @fjmolinas @benpicco @kaspar030