Implement timeouts for wait calls

[KnorpelSenf]

Each conversation can now time out. The timeouts can be set on two levels:

1. when calling createConversation, will be used for all conversations with that identifier
2. inside a conversation by passing a value to wait, overrides 1

This feature works on the time per wait call. If the wait call takes longer than the specified time, it will be left automatically.

There is no way to set a timeout for the entire conversation.

Closes #18.

[codecov-commenter]

Codecov Report

Base: 96.48% // Head: 96.45% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (bab72f7) compared to base (09a4fe9). Patch coverage: 98.14% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
- Coverage   96.48%   96.45%   -0.03%     
==========================================
  Files           4        4              
  Lines         825      847      +22     
  Branches      119      124       +5     
==========================================
+ Hits          796      817      +21     
- Misses         28       29       +1     
  Partials        1        1              

Impacted Files	Coverage Δ
src/utils.ts	94.44% <ø> (-0.16%)	:arrow_down:
src/conversation.ts	96.12% <98.14%> (-0.02%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[yn4v4s]

I can imagine that it is desirable to have a timeout that regard each wait call individually, rather than seeing the conversation in its entirety. Should this be changed before merging?

Closes #18.

It would be nice to have both, a timeout for the entire conversation and individual timeouts for each wait call, but right now conversation timeout seems more useful IMO

[yn4v4s]

I'm wondering how will this work on serverless environments that scale to cero, like cloud functions. What would happen if the instance (or function) handling this convo goes to sleep before the timeout expires.

[KnorpelSenf]

right now conversation timeout seems more useful IMO

Alright, then we can leave this as-is. I hope that #11 will allow us to easily add wait-specific timeouts later on. Also, users are able to adjust the timeout on the fly as they go from wait call to wait call, hence effectively using a wait-specific timeout.

I'm wondering how will this work on serverless environments that scale to cero, like cloud functions. What would happen if the instance (or function) handling this convo goes to sleep before the timeout expires.

This is naturally supported. If you look at the changes, you can see that there is no use of setTimeout or other measures that would fail if the instance was restarted between updates. Timeouts are purely implemented through timestamps and they work on a per-update basis. Similar to the existing session timeouts that can expire the entire session, conversation timeouts work in all environments, including serverless ones.

Nearly all grammY code is written to support all platforms. The only exceptions to this might be conversation.sleep which uses setTimeout (but it should probably not be used at all anyway), and sequentialize which is unable to order updates correctly that are sent to different instances in a concurrent environment (but its purpose is to be used with grammY runner anyway).

[mikerockett]

… Also, users are able to adjust the timeout on the fly as they go from wait call to wait call, hence effectively using a wait-specific timeout.

I’m happy with this for the time being. Tiny bit of reused code to add that can be removed easily later.

Other than that, this works great!

[KnorpelSenf]

Perhaps we could simply add an option to the wait calls with a timeout? That would be very easy to implement. I don't know why I didn't see this earlier …

Perhaps timeoutAfterMilliseconds to be clearer?

Sure, that would work, too. I can change it. Or maybe just timeoutMilliseconds?

[mikerockett]

Perhaps we could simply add an option to the wait calls with a timeout? That would be very easy to implement. I don't know why I didn't see this earlier …

Assuming that has the same effect as assigning it manually in-conversation (level 3), I don't see why not.

Sure, that would work, too. I can change it. Or maybe just timeoutMilliseconds?

Up to you of course – I just like to make things as crystal-clear / expressive as possible.

[KnorpelSenf]

Perhaps we could simply add an option to the wait calls with a timeout? That would be very easy to implement. I don't know why I didn't see this earlier …

Assuming that has the same effect as assigning it manually in-conversation (level 3), I don't see why not.

Now that I think about it, it would be confusing to do wait({ timeoutAfterMilliseconds: 5000 }) and then have this affect the timeout of the next wait call. I think if we implement wait-specific timeouts, we should do it right, and have them be independent from conversation-level timeouts.

As I understand it, we definitely want to have wait-specific timeouts.

Do we really need to have conversation-specific timeouts? This looks like a lot of timeouts, and I am not sure if they clutter the API more than they help.

For example, perhaps we should only have wait-specific timeouts, but then be able to set level 1 and level 2 defaults for them? For example, doing

await ctx.conversation.enter("convo", { doNotEverWaitAnyLongerThanMillisecondsOkayThisWillBeRenamed: 5000 })

which is level 2 would make all wait calls timeout after 5 seconds.

Is that a cleaner way of implementing this feature maybe?

[mikerockett]

I’m actually in agreement with this. For my own use-case, I can’t think of a reason why I would want to limit the time it takes to have an entire conversation, but I would want to discard conversations that have been inactive for some time, and that can only be determined by setting timeouts at wait-level (no matter what level they’re actually configured at).

Separately from this, I would like to suggest a callback be added to the config, such that the bot can respond to a now-dead conversation. Something like this:

{
  doNotEverWaitAnyLongerThanMillisecondsOkayThisWillBeRenamedBecauseItsJustSuchALongNamePhew: 300000,
  onTimeout: async context => await context.reply(
    'My memory’s not so good because of that long name. You’ll need to start over…'
  ),
}

This seems possible given that conversations are “destroyed” lazily.

[KnorpelSenf]

I’m actually in agreement with this.

Nice, then I'll change the conversation-specific timeouts to be wait-specific everywhere.

Separately from this, I would like to suggest a callback be added to the config, such that the bot can respond to a now-dead conversation. Something like this:

{
  doNotEverWaitAnyLongerThanMillisecondsOkayThisWillBeRenamedBecauseItsJustSuchALongNamePhew: 300000,
  onTimeout: async context => await context.reply(
    'My memory’s not so good because of that long name. You’ll need to start over…'
  ),
}

This seems possible given that conversations are “destroyed” lazily.

The timeout handler will not trigger after 300000 milliseconds. It will only fire upon the first new update that arrives, which could potentially be a year later.

I don't mind adding the feature, but is that what is desired? If yes, how can we communicate it to library users that onTimeout with a 5 seconds timeout does not run after 5 seconds?

[mikerockett]

The timeout handler will not trigger after 300000 milliseconds. It will only fire upon the first new update that arrives, which could potentially be a year later.

Exactly what I'm aiming for.

I don't mind adding the feature, but is that what is desired? If yes, how can we communicate it to library users that onTimeout with a 5 seconds timeout does not run after 5 seconds?

Perhaps just a better name – onTimeout is probably not a good one. Maybe afterTimeout?

Haha, naming things is hard. 🤪

[KnorpelSenf]

I’m actually in agreement with this.

Nice, then I'll change the conversation-specific timeouts to be wait-specific everywhere.

This would lead to heavy clashes with #51 which also changes the function signatures of wait calls. I will implement it after #51 is merged.

The timeout handler will not trigger after 300000 milliseconds. It will only fire upon the first new update that arrives, which could potentially be a year later.

Exactly what I'm aiming for.

Amazing! Let's do it then 🚀

[KnorpelSenf]

This can be advanced now

[KnorpelSenf]

This looks a lot like otherwise would be called if the user does not respond within 5000 ms. What can we do here to make sure that people don't fall into this trap?

[KnorpelSenf]

@KnightNiwrem I'd love to get feedback on the API. You can now pass timeout values to createConversation as well as all wait* calls. Is this sane?