Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

gcoap: fixes around empty ACKs [backport 2022.07] #18436

Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Aug 10, 2022

Backport of #18429

Contribution description

This adds two fixes around the handling and sending of empty ACKs:

  1. Send empty ACKs on CON response, even if there is no memo that matches this response.
  2. Expire request on empty ACK when there is no response handler to wait for.

The 1. fix may be disputable. In my opinion it makes more sense to send a few short empty ACKs more than having some server sending a potentially large CON response multiple times just because the server missed a previous ACK, but I can remove this if this is for some reason not a good idea.

Testing procedure

Not sure how to test this... I saw it while running some experiments with #18386.

Issues/PRs references

None, but improves behavior in #18386.

@miri64 miri64 added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 10, 2022
@miri64 miri64 enabled auto-merge August 11, 2022 09:14
@chrysn
Copy link
Member

chrysn commented Aug 11, 2022

In my understanding this mainly a performance and protocol correctness (which we generally don't test extensively) fix; fine to have it backportet, but I wouldn't issue another RC for this if nothing else comes up that warrants one.

@miri64
Copy link
Member Author

miri64 commented Aug 11, 2022

In my understanding this mainly a performance and protocol correctness (which we generally don't test extensively) fix; fine to have it backportet, but I wouldn't issue another RC for this if nothing else comes up that warrants one.

#18433 and #18434 might also be valid backports. But either way, yes: I don't think it makes much sense to warrant an RC for either of those. Just go into release once those backports are merged.

@miri64 miri64 merged commit c0ebbc1 into RIOT-OS:2022.07-branch Aug 11, 2022
@miri64 miri64 deleted the backport/2022.07/gcoap/fix/empty-ack branch August 11, 2022 13:52
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master 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.

3 participants