Don't merge yet: Keep on listening to Telegram's updates even if requests with telegram server fail #40

2018-02-16T00:28:36Z

laurynas-karvelis commented

2018-02-16 00:28:36 +00:00

(Migrated from github.com)

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?

mdibaiee commented

2018-02-16 04:56:53 +00:00

(Migrated from github.com)

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

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

laurynas-karvelis commented

2018-02-16 12:10:04 +00:00

(Migrated from github.com)

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.

mdibaiee commented

2018-02-16 18:58:43 +00:00

(Migrated from github.com)

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:

laurynas-karvelis commented

2018-02-17 12:44:10 +00:00

(Migrated from github.com)

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.

mdibaiee commented

2018-02-17 12:46:31 +00:00

(Migrated from github.com)

@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)`

laurynas-karvelis commented

2018-02-17 12:48:09 +00:00

(Migrated from github.com)

Sweet, I'll do a PR update today or tomorrow then if you'd like.

👍 1

mdibaiee commented

2018-02-17 18:22:41 +00:00

(Migrated from github.com)

@laurynas-karvelis that would be awesome, thanks!

laurynas-karvelis commented

2018-02-18 10:41:46 +00:00

(Migrated from github.com)