Timeout for bot.tabComplete()#3293
Conversation
| const [packet] = await once(bot._client, 'tab_complete') | ||
| // Doing promise race, because of 'tab_complete' event never happening, | ||
| // in situation when there was no completions for tabbed command. | ||
| const eventPromise = once(bot._client, 'tab_complete') |
There was a problem hiding this comment.
We already have an onceWithCleanup function that handles timeouts. You should use that instead here.
There was a problem hiding this comment.
I think now is good?
| @@ -1,4 +1,4 @@ | |||
| const { once } = require('events') | |||
| const { onceWithCleanup } = require('mineflayer/lib/promise_utils') | |||
There was a problem hiding this comment.
require('../promise_utils')
| } | ||
|
|
||
| async function tabComplete (text, assumeCommand = false, sendBlockInSight = true) { | ||
| async function tabComplete (text, assumeCommand = false, sendBlockInSight = true, timeout = 100) { |
There was a problem hiding this comment.
100ms seems rather low of a timeout for high latency connections
There was a problem hiding this comment.
Then someone can change it for their use case. But if you think that e.g. 200 would be better, then I can change it
There was a problem hiding this comment.
How about now? It's based on bot's ping
There was a problem hiding this comment.
No, that's worse. Ideally we stick to something at least 1 second, high latency connections are perfectly valid, they should not be an error case. Also, we shouldn't have every function having inconsistent timeout values, that makes things very messy.
I just checked the mineflayer code for existing timeouts, for blocks.js and command blocks and uses 5000ms timeout, so I'd say use that here also.
There was a problem hiding this comment.
I don't see why dynamic timeout would be worse, but alright, I will change it to 5s
extremeheat
left a comment
extremeheat
left a comment
There was a problem hiding this comment.
LGTM, but I'm not sure returning an empty array is any better than letting an exception propagate to the user
| const [packet] = await onceWithCleanup(bot._client, 'tab_complete', { timeout }) | ||
| return packet.matches | ||
| } catch { | ||
| return [] |
There was a problem hiding this comment.
Yes I think let's not try catch here. The user should handle the error appropriately (for example maybe by retrying)
3 participants
I added a timeout, because without it, a promise was never* resolved, if provided command/string had no completions.
*Never, meaning till next successful tab completion, when it would just double the newer result.
I set the default timeout to, what I think is enough low/high, 100 ms. But it can be changed with a function argument, so everyone will set whatever they want.