Skip to content

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

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:gnrc_tcp-fix_recv_conn_closed
Oct 29, 2019
Merged

gnrc_tcp: return immediatly on gnrc_tcp_recv if a connection is closing#12367
miri64 merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:gnrc_tcp-fix_recv_conn_closed

Conversation

@brummer-simon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

brummer-simon commented Oct 5, 2019

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

Better now?

@brummer-simon
Copy link
Copy Markdown
Member Author

brummer-simon commented Oct 5, 2019

@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
Copy Markdown
Member Author

brummer-simon commented Oct 6, 2019

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
Copy Markdown
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
Copy Markdown
Member

miri64 commented Oct 7, 2019 •
edited
Loading

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

@brummer-simon
Copy link
Copy Markdown
Member Author

brummer-simon commented Oct 17, 2019

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

@brummer-simon
Copy link
Copy Markdown
Member Author

brummer-simon commented Oct 22, 2019

@miri64 - Ping due to force push.

miri64
miri64 approved these changes Oct 22, 2019
View reviewed changes
Copy link
Copy Markdown
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

miri64
miri64 requested changes Oct 22, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author

brummer-simon commented Oct 22, 2019

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

Done

@brummer-simon
Copy link
Copy Markdown
Member Author

brummer-simon commented Oct 29, 2019

@miri64 - Ping

miri64
miri64 approved these changes Oct 29, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

fjmolinas commented Jan 21, 2020

@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
Copy Markdown
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
Copy Markdown
Contributor

fjmolinas commented Jan 21, 2020

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