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

Observe cancellation by client #195

Closed
mkovatsc opened this issue Sep 24, 2018 · 13 comments
Closed

Observe cancellation by client #195

mkovatsc opened this issue Sep 24, 2018 · 13 comments

Comments

@mkovatsc
Copy link

node-coap currently does not support proactive cancellation via GET with Observe=1.

In server.js, the _handle() function should probably try to find the corresponding ObserveStream via the token when request.headers['Observe'] === 1 instead of producing a new response with response = new Message(packet, function(response, packet). The ObserveStream needs to be closed. Sending the last (cached) response/notification with that token should work; otherwise, ObserveStream could also take a callback to produce a final response.

@lucarv
Copy link

lucarv commented Jan 3, 2020

I wonder how to implement this correctly. According to (my perhaps poor) understanding of the specs, when the client stop sending ack to a data posted event, the server should then close the stream. I am not sure how to stop the client from sending an ack to the data event. Is it even possible?

@sjlongland
Copy link
Contributor

Well, no, not when it stops sending ACKs… that'll just cause the server to re-send the notification until it exhausts the retry count.

There are two ways:

  • Client replies to the next notification with RST.
  • Client sends a GET with token matching the original request and Observe=1.

node-coap does the former when you close the stream, it just cleans up the request at the client end, so next notification it naturally responds with the RST message. To be proactive about things, it just needs to generate that GET request at that time instead.

@GiedriusM
Copy link
Collaborator

Per RFC7641 the default way of handling a canceled observation is by sending RST:

A client that is no longer interested in receiving notifications for a resource can simply "forget" the observation.`

and the active cancellation is optional:

a client MAY explicitly deregister by issuing a GET request that has the Token field set to the token of the observation to be cancelled and includes an Observe Option with the value set to 1 (deregister).

A flag to the close method could be added to indicate active/forced cancellation:

ObserveReadStream.prototype.close = function(sendCancel) {
  if (sendCancel) {
    // duplicate original req and send with observe=1 ...
  }
  this.push(null)
  this.emit('close')
}

However,

... All other options MUST be identical to those in the registration request except for the set of ETag Options.

and the whole request option duplication may look a bit messy/out-of-context in this place.

Your thoughts?

@lucarv
Copy link

lucarv commented Jan 3, 2020

@sjlongland my bad, i should have re-read the RFC instead of just googling it ;) I found something here and believed it blindly. Anyway, I suppose the lib does not support Observe set to false, and the reset() crashes (there is an issue on it). I found a way around anyway, thanks for the reply.

@sjlongland
Copy link
Contributor

@lucarv Well no, Observe should always be set to an unsigned integer value, never a boolean (e.g. false). Specifically, 0 if you're subscribing, 1 if you're unsubscribing… or any 24-bit value if you're the server sending a notification (the value just has to be slightly greater than the last, modulo 24-bits).

@GiedriusM I don't think duplicating the options is a big issue. If it's got to be done, it's got to be done. It's either that, or tie up a subscription slot on a CoAP server (which could be a tiny embedded device). Waiting for the next notification relying on the RST being sent was a right nuisance in my experience when the server can only handle a single subscriber at a time.

To me, leaving servers with tiny amounts of RAM tied up in a subscription we're not interested in is a tardy way to handle it. Reasonable if the client crashed and subsequently forgot about its subscription, but not good otherwise. If the client knows it won't need something, it should free that resource for someone else.

@lucarv
Copy link

lucarv commented Jan 4, 2020 via email

@stale
Copy link

stale bot commented Jul 13, 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 within the next 7 days. Please check if the issue is still relevant in the most current version of the adapter and tell us. Also check that all relevant details, logs and reproduction steps are included and update them if needed. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 13, 2020
@sjlongland
Copy link
Contributor

Activity has been taking place in pull requests. See PR #164 and PR #214.

@stale stale bot removed the wontfix label Jul 14, 2020
@stale
Copy link

stale bot commented Jul 28, 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 within the next 7 days. Please check if the issue is still relevant in the most current version of the adapter and tell us. Also check that all relevant details, logs and reproduction steps are included and update them if needed. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 28, 2020
@sjlongland
Copy link
Contributor

PR #164 is closed, PR #214 is in review.

Issue would be fixed if PR #214 was merged: I therefore don't understand the wontfix label.

@stale stale bot removed the wontfix label Jul 28, 2020
@Apollon77
Copy link
Collaborator

That comment was completely correct. Stalebot can only check last activity on an issue

@sjlongland
Copy link
Contributor

I think with the merge of PR #214 this issue is now fixed.

@JKRhb
Copy link
Member

JKRhb commented Jun 15, 2021

I think with the merge of PR #214 this issue is now fixed.

You are right, I'll close this issue. Thanks once again!

@JKRhb JKRhb closed this as completed Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants