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

net/gcoap: Confirmable Observe notifications #7548

Closed
wants to merge 4 commits into from

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Aug 31, 2017

This PR implements the fourth step in the confirmable messaging plan in #7192, and depends on #7337. It allows for an Observe notification to be sent confirmably. Also, the server may deregister a client from further notifications based on the response. The client is deregistered if it responds to the notification with a reset (RST) message, or if the server does not receive a response from the client. This PR also includes refactoring of a couple of utility methods to support the implementation.

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Aug 31, 2017
@kb2ma kb2ma changed the title net/gcoap: Confirmable notifications net/gcoap: Confirmable Observe notifications Aug 31, 2017
@kb2ma kb2ma mentioned this pull request Aug 31, 2017
6 tasks
@smlng smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 14, 2017
@kYc0o kYc0o mentioned this pull request Oct 30, 2017
8 tasks
@smlng smlng removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 16, 2018
@smlng
Copy link
Member

smlng commented Jan 16, 2018

#7337 is merged, please rebase

@kb2ma
Copy link
Member Author

kb2ma commented Jan 26, 2018

Rebased.

* via a reset (RST) response to a non-confirmable notification.
* the Observe option value set to 1. A confirmable notification also is
* cancelled by a reset (RST) response from the client or by the absence of an
* ACK response.
Copy link
Member

@miri64 miri64 Mar 28, 2018

Choose a reason for hiding this comment

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

Is this according to the standard? What if the ACK (or the CON for that matter) just gets lost?

Copy link
Member Author

@kb2ma kb2ma Apr 2, 2018

Choose a reason for hiding this comment

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

Yes, according to RFC 7641. There is a short summary at the end of Sec 1.2:

When the client deregisters, rejects a notification, or the transmission of a notification
times out after several transmission attempts, the client is considered no longer
interested in the resource and is removed by the server from the list of observers.

See also Sec 4.5 specifically on retransmissions. This removal of a client is a MUST (in the RFC 2119 sense).

Observe is meant to be a best-effort attempt to notify the client. It's not a guaranteed delivery mechanism. However, the server can include a Max-Age option in the notification so the client can decide to re-register for the resource if it hasn't heard from the server.

The code currently does not include a Max-Age option. See #8831 for the plan to make it easy to add arbitrary options to a message.

Copy link
Member

Choose a reason for hiding this comment

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

[…] or the transmission of a notification times out after several transmission attempts, […]

How many retransmission attempts are there in this implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the quote from sec. 4.5:

The client's acknowledgement of a confirmable notification signals
that the client is interested in receiving further notifications. If
a client rejects a confirmable or non-confirmable notification with a
Reset message, or if the last attempt to retransmit a confirmable
notification times out, then the client is considered no longer
interested and the server MUST remove the associated entry from the
list of observers.

So, the number of retransmissions is governed by the confirmable retransmission count in the implementation. nanocoap sets COAP_MAX_RETRANSMIT to 4, so 5 transmission attempts are made altogether, using exponential backoff (2, 4, 8, 16 seconds).

You also might be interested to read at the end of sec. 4.5 about non-confirmable transmission. If notifications are sent non-confirmably, at least once a day the implementation MUST send a transmission confirmably. So, the spec uses the confirmable retransmission limit to eliminate Observe clients no longer interested in the resource.

This requirement to send a message confirmably at least once a day is not enforced by gcoap.

@miri64
Copy link
Member

miri64 commented Mar 28, 2018

Needs a rebase.

Supports handling an empty message response.
Supports new uses where a coap_pkt_t PDU is not available.
Handles if no ACK response, or if a reset (RST) response.
@kb2ma
Copy link
Member Author

kb2ma commented Apr 4, 2018

Rebase looks good.

@smlng smlng self-requested a review April 10, 2018 15:35
@bergzand bergzand added the Area: CoAP Area: Constrained Application Protocol implementations label May 25, 2018
@miri64
Copy link
Member

miri64 commented Jan 9, 2019

Sorry, this somehow got lost. @kb2ma can you rebase again, please?

@kb2ma
Copy link
Member Author

kb2ma commented Jan 10, 2019

Thanks for circling back on this one, @miri64. It's been quite a while since the original work, so I'd like to review this myself as well. However, my head is deeply in #10732 at the moment. Let me start generating PRs for that, and then I'll work on this one.

@miri64
Copy link
Member

miri64 commented Jan 10, 2019

Alright.

@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@kb2ma kb2ma removed the State: stale State: The issue / PR has no activity for >185 days label Aug 12, 2019
@stale
Copy link

stale bot commented Feb 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Feb 11, 2020
@kb2ma kb2ma added the State: don't stale State: Tell state-bot to ignore this issue label Feb 11, 2020
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Feb 11, 2020
@kb2ma kb2ma closed this Jun 16, 2020
@miri64 miri64 added State: archived State: The PR has been archived for possible future re-adaptation and removed State: don't stale State: Tell state-bot to ignore this issue labels Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking State: archived State: The PR has been archived for possible future re-adaptation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants