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

Potentially insufficient error handling for emitEvent() #256

Closed
jak0bw opened this issue Aug 28, 2020 · 5 comments
Closed

Potentially insufficient error handling for emitEvent() #256

jak0bw opened this issue Aug 28, 2020 · 5 comments

Comments

@jak0bw
Copy link

jak0bw commented Aug 28, 2020

Related to #237 but on the sender side.

When emitting an event §8.27 The emitEvent() method does only define a SecurityError and a NotFoundError, but does not define an error if the emit fails due to another, perhaps technical problem. Consider similar to #237 a sender who wants to emit an event to the configured, but currently offline mqtt broker. This probably results in an error on the protocol level, but is never communicated to the application level. EmitEvent() probably needs a similar definition as:

§7.13 The invokeAction() method
8. If the request fails locally or returns an error over the network, reject promise with the error received from the Protocol Bindings and abort these steps.

zolkis added a commit to zolkis/wot-scripting-api that referenced this issue Sep 14, 2020
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
zolkis added a commit that referenced this issue Sep 23, 2020
Fix #237 and #256: handle subscription errors
@zolkis
Copy link
Contributor

zolkis commented Sep 28, 2020

The PR #264 is not fixing the issue. Discussion summary from the Scripting call on 28.9.2020.
The problem has many sides:

  1. The ExposedThing implemention script should get an error when a notification fails (that is, local delivery). This should be standardized. I can make another PR for this.
  2. If there is protocol (binding) support for conveying a subscription error to the clients, then it could be signaled at the clients through the ErrorListener callback. This could be implementation specific, until the TD spec defines a generic error mechanism for this.
  3. The TD could define the errors to be signaled when interactions fail, or it could define specific interactions to handle error states (e.g. properties or actions or events). So, it's not standardized in the TD spec, but errors are considered to be business logic, to be defined in the TDs.
  4. There was a point about handling errors at the client side for subscriptions (both via Promise and the error callback). This needs to be clarified more.

@egekorkan, @sebastiankb, @mjkoster please advise.

@zolkis
Copy link
Contributor

zolkis commented Sep 30, 2020

  1. The ExposedThing implemention script should get an error when a notification fails (that is, local delivery). This should be standardized. I can make another PR for this.

The proposed solution for this one would be to return a Promise by emitEvent(), which might reject with an error when the event notification is not successful. This is a trivial change.

  1. If there is protocol (binding) support for conveying a subscription error to the clients, then it could be signaled at the clients through the ErrorListener callback.

For this one the Scripting API already has coverage via ErrorListener, though the exact mechanism is encapsulated by implementations and might differ binding from binding.

  1. There was a point about handling errors at the client side for subscriptions (both via Promise and the error callback).

This is still outstanding and could be discussed in a separate issue. One (quite trivial) solution would be to revert to the separate subscribe()-unsubscribe() and observe()-unobserve() pair, which also align better with the TD spec. On subscription, a subscription id could be returned to ease subscription management.

@danielpeintner
Copy link
Contributor

see #269

@danielpeintner
Copy link
Contributor

Scripting Call 2020-10-19.
Closing...

TD related issue "defining error"

@relu91
Copy link
Member

relu91 commented Oct 26, 2020

Open point:

The TD could define the errors to be signaled when interactions fail, or it could define specific interactions to handle error states (e.g. properties or actions or events). So, it's not standardized in the TD spec, but errors are considered to be business logic, to be defined in the TDs.

I am closing this one, the issue was solved and the last open point it is being tracked on the following issues:

@relu91 relu91 closed this as completed Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants