-
Notifications
You must be signed in to change notification settings - Fork 28
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
Report protocol errors that occur after subscriptions to Events and Property updates #237
Comments
See comment. |
Changed the name, since it was suggesting a solution that needs debate. The solution proposal was this: extend However, the API design has moved away from that Node.js pattern towards idioms used in browser APIs. Another proposal is to extend Also, we need to extend the algorithms, for instance add error handling to the |
In the Scripting call today, we discussed the following options:
People in the call looked to prefer 3 - whether a. or b. that was not discussed, but I assume 3a was in the scope of discussions. |
There is a problem though with 3. and 4. above, namely that we've tried to avoid DOM Events for a reason (among which the fact that the reference implementation is node-wot). So there is a 5th option, to use a subscription object very much like to the ThingDiscovery interface. The partial interface ConsumedThing {
Promise<ThingSubscription> observeProperty(DOMString name,
optional InteractionOptions options = null);
Promise<ThingSubscription> subscribeEvent(DOMString name,
optional InteractionOptions options = null);
}; then we can define [SecureContext, Exposed=(Window,Worker)]
interface ThingSubscription {
Promise<InteractionOutput> next();
undefined cancel();
readonly attribute boolean active;
readonly attribute Error? lastError;
}; or even simpler, [SecureContext, Exposed=(Window,Worker)]
interface ThingSubscription {
Promise<InteractionOutput> next(); // also conveys errors
undefined cancel();
}; EDIT: of course we could have separate PropertyUpdateSubscription and EventSubscription declarations. |
Option 6 would be to change the algorithm for observe() and subscribe() - as suggested in the original issue, namely that they would only resolve when canceled (unobserve() / unsubscribe() is called) and would reject when an irrecoverable error occurs in the subscription. However that is not the intended use of Promises AFAICT. Based on offline discussions so far it seems 3b is the most preferred option (but using EventTarget, not Node's EventEmitter), maybe a more refined option 5 is still in the play. |
I like this most. It also seems very close to "RxJS Subscriptions". Mhh, I did not have time to explore/implement it any further ... this might help also I think.
If there is no need I would not do so (at least I do not see any reason for doing that at the momemnt). |
FYI: node-wot internally still uses rxjs Subscription, see here |
So let's have examples: try {
let observe = await thing.observe("temperature");
while (observe.active) {
let value = await observe.next();
}
} catch (e) {
// ... log: observing "temperature" failed
}; I'd argue we need at least the try {
let subscription = await thing.subscribe("temperaturechange");
while (!subscription.error) {
let value = await subscription.next();
}
} catch (e) {
// ... log: event subscription for "temperaturechange" failed
}; While the above seems okay for Property observes, it feels awkward for Events. Compare with this: let subscription = thing.subscribe("temperaturechange");
subscription.onchange = (value) => { ... };
subscription.onerror = e => { ... };
subscription.start();
// ...
subscription.stop(); |
Some more generic comment:
w.r.t. code I envision something like we have in node-wot // I can pass along next and error handlers
let subscription = thing.subscribe("temperaturechange", next, error);
// once I am not interested any longer I call unsubscribe
subscription.unsubscribe(); I am not event sure we need something like active/closed? Another listener could be also Does that that look better for you or are we turning in circles now ;-) |
If we put callbacks in the observe()/subscribe() signature then it makes little sense to create a subscription object, then we could keep the unobserve()/unsubscribe() method. Using callback + error callback in the signature is kind of lame. Also, using So let's see my preferences:
We're used with going in circles... and that is fine, if the radius is diminishing :). |
I'm answering to various points presented above:
I do not think that this solution has the same cons as the first one. If we add another callback the developer should not check if the error is defined or do other controls. He just provides two functions. If the error is
On the other hand, it is exactly how RxJs Subscription is designed. Not saying that we have to blindly follow that design but just to clarify a little bit for the discussion: RxJsObservable.subscribe(
res => console.log('HTTP response', res),
err => console.log('HTTP Error', err),
() => console.log('HTTP request completed.')
); We can do the same as mentioned in the daniel's comment: // First variant use function complete to know if the subscription was accepted (?)
let subscription = thing.subscribe("temperaturechange",
async (res) => console.log('Temperature changed', await res.value()),
err => console.log('Subscription connection error', err),
() => console.log('Subscribed')
);
// once I am not interested any longer I call unsubscribe
subscription.unsubscribe();
// Second variant remove the complete and matain subscribe to be async
// i.e. resolve only if the subscription was accepted (websocket connected, mqtt client connected ...)
let subscription = aync thing.subscribe("temperaturechange",
async (res) => console.log('Temperature changed', await res.value()),
err => console.log('Subscription connection error', err),
);
// once I am not interested any longer I call unsubscribe
subscription.unsubscribe(); Finally, solution 3b or 3a might look promising. However, we cannot create the subscription asynchronously. Following that solution a subscription will look something like this: // note: we cannot have an async creation of subscription
let subscription = async thing.subscribe("temperaturechange");
subscription.onsubscribe = (data)=>{/*do something*/}
subscription.ondata = (data)=>{/*do something*/}
subscription.onerror = ()=>{/*do something*/}
subscription.unsubscribe = ()=>{/*do something*/}
// once I am not interested any longer I call unsubscribe
subscription.unsubscribe();
👍 |
We need to decide the behaviour of a subscription. Using 2 callbacks, after subscription one can get either notifications or errors, mixed as pleased. That is not a good representation of subscriptions (of course the algorithm may say that after error there are no more notifications if the error was non-recoverable, but still every little error will be popping up and that's not what the subscriber is after. IMHO a subscriber is interested mainly in the notifications, or otherwise to know they will never come - eventually why they are not coming. That should be taken into account when designing the API + the algorithm. Currently what we are missing is the "sorry, notifications won't come, there is an error". If we only allow critical error reporting (subscription stopped after error), then we can go along with 1) an error event 2) or callback, or 3) status + last error. If we allow non-critical error reporting as well, then we need the If we choose the RxJS design, I'd prefer returning a Promise for the subscribe call and then the callback + error callback are added to the subscription as internal slots. If it was a browser API, this would be the recommended solution: try {
const controller = new AbortController();
let subscription = await thing.subscribe("temperaturechange", { signal: controller.signal } );
subscription.onchange = data => { ... };
subscription.onerror = error => { ... };
// stop subscription after 60 seconds
setTimeout(() => controller.abort(), 60000);
} catch (e) {
// ... subscription failed
} Using AbortController would enable canceling multiple subscriptions with one call. |
It probably does not affect the currently pending decision but I want to add a consideration to this statement:
If the subscriber can choose between different subscription providers (e. g. different sensors for redundancy that ultimately measure the same thing) and he experiences non critical connection problems, he still might want to change his subscribtion to another one of those redundant subscription providers and thus perhaps wants to know about a non fatal connection error. |
If we need to convey both critical and recoverable errors, then we need to add status to subscriptions.
|
My example might be too rare/not "viable enough" to deserve any adaptions in the Scripting API design. In the end if the subscription recovered it still should work and the Thing can switch its subscription provider when a fatal error occurred... I just wanted to add this thought to the conversation and let the more experienced people decide if this or another similar use case is worth considering. |
The question always is - what are the use cases. Since quite valid use cases have been presented, we will discuss how to adapt the API, on next Monday's call. |
Scripting Call Aug 31 - Summary
Please comment if summary is not appropriate @zolkis @relu91 |
see discussions in eclipse-thingweb/node-wot#232 (comment)
The text was updated successfully, but these errors were encountered: