worker timeouts #7
Conversation
|
Easy way to get around the 0 timeout is to have a minimum if one is defined. You could use the check like so: if (!isNumber(timeout) || timeout < process.env.MINIMUM_TIMEOUT) {
throw ...
}This way you allow the user to set the minimum timeout in a 12-factor way, while keeping things sane in case someone misses a |
| .cancellable() | ||
| .then(function () { return this.task(this.job); }); | ||
| }) | ||
| .timeout(this.msTimeout); |
There was a problem hiding this comment.
So you are always applying the timeout? I wonder if it should just not apply it if the user did not define one...
There was a problem hiding this comment.
this has been changed now. If a user specifies no timeout (or a timeout of 0), it will not set a timeout on the promise.
| }) | ||
| .then(function () { | ||
| var timeout = module.exports.globalTimeout; | ||
| module.exports.globalTimeout = module.exports.globalTimeout / 2; |
There was a problem hiding this comment.
do you cut the timeout in half every retry?
There was a problem hiding this comment.
yes, for this particular example worker. I'm thinking now that maybe I should supply a list of timeouts, and once there are no more timeouts, it throws a TaskFatalError. That could be useful...
There was a problem hiding this comment.
added more information to the description, but didn't change the implementation
|
seems good to me, but I am not an expert with promises yet. |
|
@rsandor I got around the 0 timeout thing by simply not adding the timeout if none was specified. I didn't set a default timeout any higher (or a suggested minimum), but I did add documentation about the environment variables :) |
|
This looks great! 💯 |
Now workers can be set to time out! Pretty cool, eh!? Unit and functional tests have been created for this functionality.
One interesting corner is that setting the timeout to 0 does not actually give the worker infinite time (it will fail almost immediately). Is this good? Is that what we want to do? I might be able to have the timeout added conditionally, but I just want to pose the question.Edit: I think the
0timeout thing has been resolved. See discussion below.