Finally managed to hunt down this edge case that would freeze entire telegram bot if one getUpdates() request fails.
Edit: I'm not really certain about this solution, in terms if it should exist at all.
On one side, this PR provides a convenience to the end module user by simplifying error recovery and guarantees bot's ability to continue to listen to new incomming messages.
On the other, the end user loses ability to take matters into their own hands because of always resolved promise. There could be cases when module user might want to handle connection error with telegram's server in another way.
What would be your suggestion @mdibaiee?
Finally managed to hunt down this edge case that would freeze entire telegram bot if one getUpdates() request fails.
Edit: I'm not really certain about this solution, in terms if it should exist at all.
On one side, this PR provides a convenience to the end module user by simplifying error recovery and guarantees bot's ability to continue to listen to new incomming messages.
On the other, the end user loses ability to take matters into their own hands because of always resolved promise. There could be cases when module user might want to handle connection error with telegram's server in another way.
What would be your suggestion @mdibaiee?
Hey!
Why not run the `poll` on a `finally` clause? See:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally
https://github.com/es-shims/Promise.prototype.finally
Your .then handler implementation is spot on where .then handler uses response data, .finally is generally used at the end of the promise chain, it does not receive any arguments into it's handler.
Also, you implemented a decent round-robin getUpdates listener by just recursing current promise chain. Hence, semantically .finally should not belong in poll.js file but in user's codebase that deals with bot.start() chain breakage issues instead. Even then, as a user, extending bot.start() with .finally doesn't make sense either (due to recursive promise chaining).
My suggestion is either to merge this PR and let people read about how this module deals with edge case scenario when getUpdates returns a rejection or.. completely ignore/close this PR and maybe just to add some explanation with a tiny example in the wiki how to revive bot's listener in case telegram's server decides to break apart:
// a simple strategy to deal with telegram failing to respond correctly on getUpdates() calls
function restartBot() {
bot.stop();
bot.start().catch(restartBot);
}
bot.start().catch(restartBot);
Sorry, for my autism :D, hope this sheds the light a bit more.
Your `.then` handler implementation is spot on where `.then` handler uses `response` data, `.finally` is generally used at the end of the promise chain, it does not receive any arguments into it's handler.
Also, you implemented a decent round-robin `getUpdates` listener by just recursing current promise chain. Hence, semantically `.finally` should not belong in poll.js file but in user's codebase that deals with bot.start() chain breakage issues instead. Even then, as a user, extending `bot.start()` with `.finally` doesn't make sense either (due to recursive promise chaining).
My suggestion is either to merge this PR and let people read about how this module deals with edge case scenario when `getUpdates` returns a rejection or.. completely ignore/close this PR and maybe just to add some explanation with a tiny example in the wiki how to revive bot's listener in case telegram's server decides to break apart:
```
// a simple strategy to deal with telegram failing to respond correctly on getUpdates() calls
function restartBot() {
bot.stop();
bot.start().catch(restartBot);
}
bot.start().catch(restartBot);
```
Sorry, for my autism :D, hope this sheds the light a bit more.
I see your point, I really don't think users should copy paste a snippet like the one you mentioned to handle errors, that's not user-friendly.
Let's keep this change, but also emit an error event when we catch an exception, what do you say? 😁
I see your point, I really don't think users should copy paste a snippet like the one you mentioned to handle errors, that's not user-friendly.
Let's keep this change, but also emit an `error` event when we catch an exception, what do you say? :grin:
Hi @mdibaiee, sorry for a late response. I was leaning to the same conclusion. Had a bit of thinking yesterday about a potential solution for such cases.
My idea is to add additional method to the bot class bot.onError(cb) that triggers cb function every time there's an error for any method inside api.js method. This way, bot can keep on resuming getUpdates recursion infinitely, but also a user can tap into errors and decide what to do case-by-case basis.
I will update my PR in couple of days time for your review to better understand of what do I mean.
Hi @mdibaiee, sorry for a late response. I was leaning to the same conclusion. Had a bit of thinking yesterday about a potential solution for such cases.
My idea is to add additional method to the bot class `bot.onError(cb)` that triggers `cb` function every time there's an error for any method inside `api.js` method. This way, bot can keep on resuming `getUpdates` recursion infinitely, but also a user can tap into errors and decide what to do case-by-case basis.
I will update my PR in couple of days time for your review to better understand of what do I mean.
@laurynas-karvelis Yeah, that's a good idea, but we don't need an additional method, bot is already an instance of EventEmitter, so just bot.emit('error', e). And then users can use bot.on('error', cb)`
@laurynas-karvelis Yeah, that's a good idea, but we don't need an additional method, bot is already an instance of `EventEmitter`, so just `bot.emit('error', e)`. And then users can use bot.on('error', cb)`
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Finally managed to hunt down this edge case that would freeze entire telegram bot if one getUpdates() request fails.
Edit: I'm not really certain about this solution, in terms if it should exist at all.
On one side, this PR provides a convenience to the end module user by simplifying error recovery and guarantees bot's ability to continue to listen to new incomming messages.
On the other, the end user loses ability to take matters into their own hands because of always resolved promise. There could be cases when module user might want to handle connection error with telegram's server in another way.
What would be your suggestion @mdibaiee?
Hey!
Why not run the
pollon afinallyclause? See:https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally
https://github.com/es-shims/Promise.prototype.finally
Your
.thenhandler implementation is spot on where.thenhandler usesresponsedata,.finallyis generally used at the end of the promise chain, it does not receive any arguments into it's handler.Also, you implemented a decent round-robin
getUpdateslistener by just recursing current promise chain. Hence, semantically.finallyshould not belong in poll.js file but in user's codebase that deals with bot.start() chain breakage issues instead. Even then, as a user, extendingbot.start()with.finallydoesn't make sense either (due to recursive promise chaining).My suggestion is either to merge this PR and let people read about how this module deals with edge case scenario when
getUpdatesreturns a rejection or.. completely ignore/close this PR and maybe just to add some explanation with a tiny example in the wiki how to revive bot's listener in case telegram's server decides to break apart:Sorry, for my autism :D, hope this sheds the light a bit more.
I see your point, I really don't think users should copy paste a snippet like the one you mentioned to handle errors, that's not user-friendly.
Let's keep this change, but also emit an
errorevent when we catch an exception, what do you say? 😁Hi @mdibaiee, sorry for a late response. I was leaning to the same conclusion. Had a bit of thinking yesterday about a potential solution for such cases.
My idea is to add additional method to the bot class
bot.onError(cb)that triggerscbfunction every time there's an error for any method insideapi.jsmethod. This way, bot can keep on resuminggetUpdatesrecursion infinitely, but also a user can tap into errors and decide what to do case-by-case basis.I will update my PR in couple of days time for your review to better understand of what do I mean.
@laurynas-karvelis Yeah, that's a good idea, but we don't need an additional method, bot is already an instance of
EventEmitter, so justbot.emit('error', e). And then users can use bot.on('error', cb)`Sweet, I'll do a PR update today or tomorrow then if you'd like.
@laurynas-karvelis that would be awesome, thanks!
I've updated my PR. Could you have a look at it @mdibaiee?
Thanks a lot!
published
telegram-api@2.0.0.Sweet! Thank you for the merge @mdibaiee!