BotSessionFactory + Minor BotSession.createPollerTask() Refactor

[bratinghosh]

Is your feature request related to a problem? Please describe.
The current implementation of BotSession.createPollerTask(), in longpolling, updates the lastReceivedUpdate without the knowledge of successful consumption of the updates polled. If the consumption fails, the lastReceivedUpdate is still updated to the max updateId among the received updates and on the next query, the old updates are lost (i.e. retrying is not possible).

Describe the solution you'd like
I would like to propose 3 updates:

1. The updatesConsumer should consume the updates before the lastReceivedUpdate is updated. This allows us to throw exceptions signifying the failure to consume the updates.
2. executor.scheduleAtFixedRate should be updated to executor.scheduleWithFixedDelay as we want the next poll to be fed into the executor queue after the current one is completed. This would allow for the next poll to have the lastReceivedUpdate depending on the success or failure of the updates consumption.
3. TelegramBotsLongPollingApplication.registerBot allow us to pass a BotSessionFactory which will allow us to extend the BotSession and write our own depending on specific requirements.

Describe alternatives you've considered
I considered updating the BotSession.lastReceivedUpdate directly from my application, but it is a bit hacky.

[Chase22]

I don't think this is a minor refactoring. Consider the following:

Updates maybe received as a batch, so we have multiple updates (Let's say 1-3)

The consumer might be multi threaded and asynchronous. We have a threadpool of 2

First thread consumes update 1, it takes a long time to process.
Second thread consumes update 2, quick processing it's done immediately. Consumes update 3. Also quick update, it's done.

Now the first thread fails and throws an exception. What do you set the nextUpdateId to? Do you reconsume all 3 updates? (remember updates are always consumed sequentially)

I think something like this is better implemented as an update consumer that keeps a queue of updates and buffers them for consumption, retrying if needed and using concepts like dead-letter queue and redelivery.

[Chase22]

Regarding the BotSesson factory: I don't anything speaking against it. I'd implement it as an supplier rather than a factory. Also needs consideration whether to add it as a construction parameter or a method parameter

[bratinghosh]

#1386 (comment)

In your example, we would have to reconsume all 3 updates to receive update 1.

Having a queue of updates (to fetch from the queue instead of the api itself) as buffer would not suffice as we still need to guarantee that all updates received through the api are being saved to the buffer. If there is an exception during this process of saving to the queue,then we are back to square one, i.e. we need to re-fetch the updates via tg api.

I believe the discussion hinges on which feature we prioritise more - not consuming duplicated updates or losing an update. And personally speaking, the handling of duplicates holds lower priority as losing an update is irreversible and the client can always have a cache to handle duplicates (if the library doesn't want to support).

[bratinghosh]

#1386 (comment)

I concur that we can implement a supplier to keep the same pattern of suppliers such as executorSupplier, backOffSupplier, etc.

[Chase22]

#1386 (comment)
If there is an exception during this process of saving to the queue,then we are back to square one, i.e. we need to re-fetch the updates via tg api.

Saving the update to the queue can be made very robust. Since there's no semantic implications here. We only need it to be a valid update (which is already the requirement for actually consuming updates). Refetching the updates brings no additional value of keeping a record ourself and resending. What i mostly wanted to point out: This is not trivial and might actually be outside the scope of the library.

The lib is a pretty thin layer surround the api. It guarantees each update is delivered at least once to the consumer. More sophisticated error handling might be scope of the implementing client. So if anything i'd offer this as a separate library that takes the place of an update consumer.

In the end is @rubenlagus the person that does the most maintenance on the library and i'd like to keep the final decision in his hands. I was considering before having more third-party libraries like ability bot that aren't actually part of this library but are linked from this repo as the central hub

[bratinghosh]

@rubenlagus any thoughts on this discussion would be appreciated.

[rubenlagus]

To be honest, I would expect that any implementation of the consume method would handle any exception/retries as required. Main reason being that the library should not block consuming updates because one of the 100 previous updates is taking longer than usual to process.

If we don't update the last received until consumption, we could get to a situation like:

• Updates 100 to 200 received
• Bot process really quick all updates except the one with id 102 spreading the work across 5 threads.
• At this point, we could easily fetch another 100 and use the idle threads to process them while ID-102 finishes.

If we block updating lastUpdate, that means your bot will be irresponsive until update 102 finish processing.

Although I tend to agree that duplication processing is probably better than missing thing, having a conversational application that freeze for all users because it can't be quick with a single user would provide way worse experience than a bot that miss 1 message from one user abut answer the other 99.

[bratinghosh]

@rubenlagus and @Chase22 I understand and see your perspective. To summarise our discussion, it is true that blocking the app for consuming certain update(s) is wasteful and slow, and as mentioned by @Chase22 we could persist the updates before we process them. These solutions are more performance inclined. I believe we can strike a middle ground and allow future devs including myself to extend or implement our own Bot session which fits the use case. The library can continue to use the default implementation that it has now, but instead of using the new keyword to create a botsession, we can inject appropriate generator. That way, we don't expect more from the library while accommodating for devs that have varied priorities for their solution.

[bratinghosh]

Unrelated to this discussion, but I have been noticing TelegramApiErrorResponseException if i leave the long polling bot running for couple of days without any updates.

Surprisingly, if i restart the application, the error goes away. Any idea if this is a persistent issue?

[Chase22]

Regarding the BotSessionFactory: As stated before: Sounds like a good idea. I'd implement it as a supplier in tandem with ObjectMapperSupplier etc.

For the second problem: Please open a new issue for that.

[rubenlagus]

As long as it is done in a backwards compatible way (by default, current behaviour shouldn't change). I don't mind a PR with this addition