shamblett / coap

A Coap package for dart
Other
16 stars 13 forks source link

Behaviour of observe in case of lost connection #151

Closed Zielscheibe closed 1 year ago

Zielscheibe commented 1 year ago

Hi

While working with the observer method of coap client, I noticed some behaviour that puzzled me.

In general, everything is fine. I get the messages from my server as expected. The interesting part begins when I shut the server down to simulate a loss of connection in my real-world scenario. I currently use the latest version 7.0.0

There are two specific points.

First

// Setup with maxRetransmit = 0 per default
final reqObs = CoapRequest.newGet()..uriPath = path;
_observer = await _client.observe(reqObs);

_observer.listen((event) {
      doStuff(event);
    }, onError: (error) {
      handleTheError();
    });

I would expect to get an error here when the connection to the server is lost. This is not the case, so it is impossible to know when to handle this situation. (Or is there any other way of detecting a connection loss I did not find?)

Second Given the basic setup from your example

try {
  final observer = await client.observe(reqObs);
  observer.listen((event) {
    doStuff(event);
  });
} catch (e) {
  handleTheError();
}

The error on a failing connection (e.g. due wrong ip address) is still thrown outside the try-catch block. I suspect that this is due to line 463 of coap_client.dart observe method, where the first response is explicitly unawaited:

unawaited(
  () async {
    final resp = await _waitForResponse(request, responseStream);
    if (!resp.hasOption<ObserveOption>()) {
      relation.isCancelled = true;
    }
  }(),
);

Maybe I got the intended usage wrong. My current workaround would be to check the connection using periodic discover requests, e.g. every 10 seconds or so. This would be feasible for my scenario since there is nothing time critical. Though, I thought I better ask. Many thanks. :)

JKRhb commented 1 year ago

Hi @Zielscheibe, thank you for opening this issue :) Thinking about your first point (and reading through the RFC once more), I got the impression that there is no real way of detecting a connection loss in the observe scenario, since the time between notifications can be arbitrarily long (one example could be a battery-powered sensor that only wakes up every 10 minutes or even every hour for 15 seconds or so to send an update). So maybe we could add an optional timeout parameter that cancels the subscription if there has not been a notification by the server within the timeout period?

Regarding your second point, I just tested removing the unawaited wrapper and the observe example still seems to work fine ā€“ the fact that exceptions from invalid requests cannot be caught at the moment seems very convincing to me to change these lines šŸ˜… What do you think about this, @JosefWN? :)

Zielscheibe commented 1 year ago

Then the first point certainly is due to my misunderstanding of the RFC definition. In any case, I stick to my idea of checking the connection every n second to handle the case of lost devices. Closing the observer client and opening it again as the device is found.

Thanks for the fast reply and clarification. :)

JosefWN commented 1 year ago

So maybe we could add an optional timeout parameter that cancels the subscription if there has not been a notification by the server within the timeout period?

We have a CoapObserveRelation.cancel() which I think would allow the library user to implement this in a few lines of code? I kind of like the idea that you had earlier, of keeping the settings for this library as closely aligned to the RFC's as possible (and in extension keeping our API's as simple as possible).

I just tested removing the unawaited wrapper and the observe example still seems to work fine ā€“ the fact that exceptions from invalid requests cannot be caught at the moment seems very convincing to me to change these lines šŸ˜… What do you think about this, @JosefWN? :)

Seems reasonable!

JKRhb commented 1 year ago

In any case, I stick to my idea of checking the connection every n second to handle the case of lost devices. Closing the observer client and opening it again as the device is found.

That sounds good :)

Thanks for the fast reply and clarification. :)

You're welcome! :)

So maybe we could add an optional timeout parameter that cancels the subscription if there has not been a notification by the server within the timeout period?

We have a CoapObserveRelation.cancel() which I think would allow the library user to implement this in a few lines of code? I kind of like the idea that you had earlier, of keeping the settings for this library as closely aligned to the RFC's as possible (and in extension keeping our API's as simple as possible).

Yeah, that is true, keeping the API as it is definitely is the better approach here :)

For the other point raised by @Zielscheibe, I opened #152 which should resolve this issue :)