From 95072a8ab237713b86f771abb81fcdb003d8be8b Mon Sep 17 00:00:00 2001 From: Brad Vogel Date: Sun, 23 Apr 2017 11:43:38 -0700 Subject: [PATCH] Change default lock timeout to 30sec. It's a more sensible default for the following reasons: * A lot of people complain about jobs being double processed currently. So there must be a lot of poorly written job processor code out there that stalls the event loop :). Or folks, like me, forget that we're running our code on tiny instances in the cloud where the CPU is so limited that a tiny bit of JS work will max the CPU. A 30sec timeout would give a bit more buffer. At least until we figure out a generic solution like #488. * An expired lock (due to event loop stalling) is quite fatal now that we check the lock prior to moving the job to the completed or failed (previously we would still move it if there wasn't a lock). So if a long running job (let's say 2min) stalls the event loop for even just 5sec, it means that the job can never complete at that point. It might still continue processing, but another worker would have likely picked it up as a stalled job and processed it again. Or if it doesn't happen to get picked up as stalled, when it finally completes it still won't be moved to completed because it lost the lock at one time. * The tradeoff is that it will take longer for jobs to be considered 'stalled'. So instead waiting max 5sec to find out if a job was stalled, we'd wait max 30sec. I think this is generally OK and that most people aren't running jobs that are that time-sensitive. Actual stalled jobs [due to process crashes] should be extremely rare anyways. This also sets the stalledInterval to 30sec since it doesn't do much good to have it run more frequently than the lock timeout. It's also slightly expensive to run in Redis as it iterates all jobs in the 'active' queue (see moveUnlockedJobsToWait.lua), so it'd be nice to run this less often anyways. --- README.md | 4 ++-- lib/queue.js | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 14d1f39a8..c64f4bf58 100644 --- a/README.md +++ b/README.md @@ -419,9 +419,9 @@ __Arguments__ // Advanced settings (see below) settings?: QueueSettings { - lockDuration?: number = 5000, + lockDuration?: number = 30000, lockRenewTime?: number = lockDuration / 2, - stalledInterval?: number = 5000, + stalledInterval?: number = 30000, maxStalledCount?: number = 1, guardInterval?: number = 5000, retryProcessDelay?: number = 5000, diff --git a/lib/queue.js b/lib/queue.js index ce3ac4cba..15ca8ed14 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -60,8 +60,9 @@ var MAX_TIMEOUT_MS = Math.pow(2, 31) - 1; // 32 bit signed // Advanced settings settings?: QueueSettings { - lockDuration?: number = 5000, - stalledInterval?: number = 5000, + lockDuration?: number = 30000, + lockRenewTime?: number = lockDuration / 2, + stalledInterval?: number = 30000, maxStalledCount?: number = 1, // The maximum number of times a job can be recovered from the 'stalled' state guardInterval?: number = 5000, retryProcessDelay?: number = 5000 @@ -140,8 +141,8 @@ var Queue = function Queue(name, url, opts){ this.retrieving = 0; this.settings = _.defaults(opts.settings, { - lockDuration: 5000, - stalledInterval: 5000, + lockDuration: 30000, + stalledInterval: 30000, maxStalledCount: 1, guardInterval: 5000, retryProcessDelay: 5000