Improve timeouts handling in conversation handlers #2417
Conversation
Signed-off-by: starry69 <starry369126@outlook.com>
Signed-off-by: starry69 <starry369126@outlook.com>
There was a problem hiding this comment.
Besides the comment I left, I'm still missing a few things:
-
We should add a note that
timeoutwithrun_asyncmay fail, if the promise takes longer than the timeout (as described in Improve timeouts handling in conversation handlers #2417) -
We need a test that makes sure the timeout handlers are not triggered, if the promise resolves to
CH.END. Ideally, we'd also have a test that makes sure that stuff still works if the promise raises an exception -
Timeout of child conversations needs to be detected by parent conversations somehow. If we can't get that to work, the added warning needs to be reformulated and the limitation needs to be documented. picking up my idea from off-line to have
check_updatecheck if the child has timed out in the meantime: that's not stright forward, as the childs state is thenNoneand also the child may be used in multiple parent conversations (having the same CH instance as child in multiple parents is surely somewhat crazy but we can't prevent that either …).TBH while typing this I feel like it may just not be worth the effort and we might just want to document the limitation for now and revisit only if someone complains …
PS: most of the test fails are due to #2409 …
Signed-off-by: starry69 <starry369126@outlook.com>
Signed-off-by: starry69 <starry369126@outlook.com>
Signed-off-by: starry69 <starry369126@outlook.com>
Imo we should leave it for now as you said, and just add a note in documentation that: using timeouts with nested conversation may cause troubles. TBH I've never seen anyone complaining about timeouts in nested conversation triggering incorrectly, even when it was bugged, which tells that users rarely use timeouts in nested conversation, besides, now we also have a warning in place so ig we should be fine... |
|
All right. I'd just to reformulate the note & warning in this case, to be more explicit. E.g. something like
for both |
Signed-off-by: starry69 <starry369126@outlook.com>
Yep sure 👍 |
|
tests now look good, but coverage is not happy. @starry69 can you check why line 647 in CH is not covered? |
starry-shivam
force-pushed
the
conv-timeout
branch
from
f7895d1
to
a2f9b91
Compare
Signed-off-by: starry69 <starry369126@outlook.com>
Fixed |
Signed-off-by: starry69 <starry369126@outlook.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: starry69 <starry369126@outlook.com>
There was a problem hiding this comment.
Thanks for your patience @starry69 ! Almost done, just some minor nit picking left :)
Signed-off-by: starry69 <starry369126@outlook.com>
|
|
||
| Note: | ||
| Using `conversation_timeout` with nested conversations is currently not | ||
| supported. You can still try to use it, but it will likely behave differently |
There was a problem hiding this comment.
| supported. You can still try to use it, but it will likely behave differently | |
| supported. You can try to use it, but it will likely behave differently |
| if isinstance(handler, self.__class__): | ||
| warnings.warn( | ||
| "Using `conversation_timeout` with nested conversations is currently not " | ||
| "supported. You can still try to use it, but it will likely behave " |
There was a problem hiding this comment.
| "supported. You can still try to use it, but it will likely behave " | |
| "supported. You can try to use it, but it will likely behave " |
Closes #2407