From b6916bbf3dd51c3d1d3281a6d4c5a589871fc4b2 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Thu, 6 Apr 2017 23:42:20 +0200 Subject: [PATCH 01/17] Implemented #190 --- lib/job.js | 9 +- lib/queue.js | 69 +++----- lib/scripts.js | 227 +++++++++++-------------- lib/scripts/index.js | 36 ++++ lib/scripts/moveToFinished.lua | 41 +++++ lib/scripts/moveToSet.lua | 42 +++++ lib/scripts/moveUnlockedJobsToWait.lua | 56 ++++++ lib/scripts/removeJob.lua | 26 +++ lib/scripts/updateDelaySet.lua | 35 ++++ test/test_job.js | 4 +- test/test_queue.js | 27 ++- 11 files changed, 384 insertions(+), 188 deletions(-) create mode 100644 lib/scripts/index.js create mode 100644 lib/scripts/moveToFinished.lua create mode 100644 lib/scripts/moveToSet.lua create mode 100644 lib/scripts/moveUnlockedJobsToWait.lua create mode 100644 lib/scripts/removeJob.lua create mode 100644 lib/scripts/updateDelaySet.lua diff --git a/lib/job.js b/lib/job.js index de4d8e335..0a3ad6f0f 100644 --- a/lib/job.js +++ b/lib/job.js @@ -180,7 +180,7 @@ Job.prototype.delayIfNeeded = function(){ Job.prototype.moveToCompleted = function(returnValue){ this.returnvalue = returnValue || 0; - return scripts.moveToCompleted(this, this.opts.removeOnComplete); + return scripts.moveToCompleted(this, this.returnvalue, this.opts.removeOnComplete); }; Job.prototype.move = function(src, target, returnValue){ @@ -218,7 +218,7 @@ Job.prototype.moveToFailed = function(err, noReleaseLock){ }); } else { // If not, move to failed - promise = _this._moveToSet('failed'); + promise = scripts.moveToFailed(_this, err.message, _this.opts.removeOnFail); } return promise.then(function(){ if(!noReleaseLock){ @@ -437,8 +437,8 @@ Job.prototype.finished = function(){ // ----------------------------------------------------------------------------- Job.prototype._isDone = function(list){ return this.queue.client - .sismember(this.queue.toKey(list), this.jobId).then(function(isMember){ - return isMember === 1; + .zscore(this.queue.toKey(list), this.jobId).then(function(score){ + return score !== null; }); }; @@ -515,7 +515,6 @@ Job.prototype._saveAttempt = function(err){ this.stacktrace.push(err.stack); params.stacktrace = JSON.stringify(this.stacktrace); - params.failedReason = err.message; return this.queue.client.hmset(this.queue.toKey(this.jobId), params); diff --git a/lib/queue.js b/lib/queue.js index 1a82afd2f..852aa4370 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -726,7 +726,6 @@ Queue.prototype.processJob = function(job){ stopTimer(); - // TODO: Should moveToFailed ensure the lock atomically in one of its Lua scripts? // See https://github.com/OptimalBits/bull/pull/415#issuecomment-269744735 job.takeLock(true /* renew */, false /* ensureActive */).then( function(/*lock*/) { return job.moveToFailed(err).finally(function(){ @@ -858,8 +857,6 @@ Queue.prototype.getJobCountByTypes = function() { switch(type) { case 'completed': case 'failed': - multi.scard(key); - break; case 'delayed': multi.zcard(key); break; @@ -890,8 +887,8 @@ Queue.prototype.getJobCounts = function(){ return this.client.multi() .llen(this.toKey('wait')) .llen(this.toKey('active')) - .scard(this.toKey('completed')) - .scard(this.toKey('failed')) + .zcard(this.toKey('completed')) + .zcard(this.toKey('failed')) .zcard(this.toKey('delayed')) .exec().then(function(result){ result.forEach(function(res, index){ @@ -902,11 +899,11 @@ Queue.prototype.getJobCounts = function(){ }; Queue.prototype.getCompletedCount = function() { - return this.client.scard(this.toKey('completed')); + return this.client.zcard(this.toKey('completed')); }; Queue.prototype.getFailedCount = function() { - return this.client.scard(this.toKey('failed')); + return this.client.zcard(this.toKey('failed')); }; Queue.prototype.getDelayedCount = function() { @@ -942,11 +939,11 @@ Queue.prototype.getDelayed = function(/*start, end*/){ }; Queue.prototype.getCompleted = function(){ - return this.getJobs('completed', 'SET'); + return this.getJobs('completed', 'ZSET'); }; Queue.prototype.getFailed = function(){ - return this.getJobs('failed', 'SET'); + return this.getJobs('failed', 'ZSET'); }; Queue.prototype.getJobs = function(queueType, type, start, end){ @@ -961,17 +958,6 @@ Queue.prototype.getJobs = function(queueType, type, start, end){ case 'LIST': jobs = this.client.lrange(key, start, end); break; - case 'SET': - jobs = this.client.smembers(key).then(function(jobIds) { - // Can't set a range for smembers. So do the slice programatically instead. - // Note that redis ranges are inclusive, so handling for javascript accordingly - if (end === -1) { - return jobIds.slice(start); - } - - return jobIds.slice(start, end + 1); - }); - break; case 'ZSET': jobs = this.client.zrange(key, start, end); break; @@ -1004,32 +990,29 @@ Queue.prototype.toKey = function(queueType){ Queue.prototype.clean = function (grace, type, limit) { var _this = this; - return new Promise(function (resolve, reject) { - if(grace === undefined || grace === null) { - return reject(new Error('You must define a grace period.')); - } + if(grace === undefined || grace === null) { + return Promise.reject(new Error('You must define a grace period.')); + } - if(!type) { - type = 'completed'; - } + if(!type) { + type = 'completed'; + } - if(_.indexOf([ - 'completed', - 'wait', - 'active', - 'delayed', - 'failed'], type) === -1){ - return reject(new Error('Cannot clean unkown queue type')); - } + if(_.indexOf([ + 'completed', + 'wait', + 'active', + 'delayed', + 'failed'], type) === -1){ + return Promise.reject(new Error('Cannot clean unkown queue type')); + } - return scripts.cleanJobsInSet(_this, type, Date.now() - grace, limit).then(function (jobs) { - _this.distEmit('cleaned', jobs, type); - resolve(jobs); - return null; - }).catch(function (err) { - _this.emit('error', err); - reject(err); - }); + return scripts.cleanJobsInSet(_this, type, Date.now() - grace, limit).then(function (jobs) { + _this.distEmit('cleaned', jobs, type); + return jobs; + }).catch(function (err) { + _this.emit('error', err); + throw err; }); }; diff --git a/lib/scripts.js b/lib/scripts.js index 36bb80174..61406fc34 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -15,9 +15,8 @@ var Redlock = require('bull-redlock'); function execScript(client, hash, lua, numberOfKeys){ var args = _.drop(arguments, 4); - debuglog(lua, args); - if(!client[hash]){ + debuglog(hash, lua, args); client.defineCommand(hash, { numberOfKeys: numberOfKeys, lua: lua }); } @@ -28,6 +27,14 @@ function isCommandDefined(client, hash){ return !!client[hash]; } +var path = require('path'); +var fs = require('fs'); +function loadScriptIfNeeded(queue, name){ + if(!queue.client[name]){ + return fs.readFileSync(path.join(__dirname, 'scripts/' + name + '.lua')).toString(); + } +} + var scripts = { _isJobInList: function(keyVar, argVar, operator) { keyVar = keyVar || 'KEYS[1]'; @@ -164,7 +171,7 @@ var scripts = { // TODO: perfect this function so that it can be used instead // of all the specialized functions moveToComplete, etc. move: function(job, src, target){ - // TODO: Depending on the source we should use LREM, SREM or ZREM. + // TODO: Depending on the source we should use LREM or ZREM. // TODO: Depending on the target we should use LPUSH, SADD, etc. var keys = _.map([ src, @@ -178,7 +185,7 @@ var scripts = { var deleteJob = 'redis.call("DEL", KEYS[3])'; var moveJob = [ - 'redis.call("SADD", KEYS[2], ARGV[1])', + 'redis.call("ZADD", KEYS[2], ARGV[3], ARGV[1])', 'redis.call("HSET", KEYS[3], "returnvalue", ARGV[2])', ].join('\n'); @@ -201,7 +208,8 @@ var scripts = { keys[1], keys[2], job.jobId, - job.returnvalue ? JSON.stringify(job.returnvalue) : '' + job.returnvalue ? JSON.stringify(job.returnvalue) : '', + parseInt(job.timestamp) ]; var returnLockOrErrorCode = function(lock) { @@ -230,11 +238,57 @@ var scripts = { } }); }, - moveToCompleted: function(job, removeOnComplete){ + _moveToCompleted: function(job, removeOnComplete){ return scripts.move(job, 'active', removeOnComplete ? void 0 : 'completed'); }, + moveToFinished: function(job, val, propVal, shouldRemove, target, shouldLock){ + var queue = job.queue; + var lua = loadScriptIfNeeded(queue, 'moveToFinished'); + + var keys = _.map([ + 'active', + target, + job.jobId], function(name){ + return queue.toKey(name); + } + ); + + var args = [ + queue.client, + 'moveToFinished', + lua, + keys.length, + keys[0], + keys[1], + keys[2], + job.jobId, + Date.now(), + propVal, + val, + shouldRemove ? "1" : "0" + ]; + + if(shouldLock){ + return wrapMove(job, function(){ + return execScript.apply(scripts, args); + }); + }else{ + return execScript.apply(scripts, args); + } + }, + + moveToCompleted: function(job, returnvalue, removeOnComplete){ + return scripts.moveToFinished(job, returnvalue, 'returnvalue', removeOnComplete, 'completed', true); + }, + + moveToFailed: function(job, failedReason, removeOnFailed){ + return scripts.moveToFinished(job, failedReason, 'failedReason', removeOnFailed, 'failed'); + }, + moveToSet: function(queue, set, jobId, context){ + var lua = loadScriptIfNeeded(queue, 'moveToSet'); + // // Bake in the job id first 12 bits into the timestamp // to guarantee correct execution order of delayed jobs @@ -264,7 +318,7 @@ var scripts = { var args = [ queue.client, 'moveToSet', - moveToSetScript, + lua, keys.length, keys[0], keys[1], @@ -280,14 +334,8 @@ var scripts = { }); }, remove: function(queue, jobId){ - var script = [ - 'redis.call("LREM", KEYS[1], 0, ARGV[1])', - 'redis.call("LREM", KEYS[2], 0, ARGV[1])', - 'redis.call("ZREM", KEYS[3], ARGV[1])', - 'redis.call("LREM", KEYS[4], 0, ARGV[1])', - 'redis.call("SREM", KEYS[5], ARGV[1])', - 'redis.call("SREM", KEYS[6], ARGV[1])', - 'redis.call("DEL", KEYS[7])'].join('\n'); + + var lua = loadScriptIfNeeded(queue, 'removeJob'); var keys = _.map([ 'active', @@ -303,8 +351,8 @@ var scripts = { var args = [ queue.client, - 'remove', - script, + 'removeJob', + lua, keys.length, keys[0], keys[1], @@ -384,27 +432,7 @@ var scripts = { top of the wait queue (so that it will be processed as soon as possible) */ updateDelaySet: function(queue, delayedTimestamp){ - var script = [ - 'local RESULT = redis.call("ZRANGE", KEYS[1], 0, 0, "WITHSCORES")', - 'local jobId = RESULT[1]', - 'local score = RESULT[2]', - 'if (score ~= nil) then', - ' score = score / 0x1000 ', - ' if (score <= tonumber(ARGV[2])) then', - ' redis.call("ZREM", KEYS[1], jobId)', - ' redis.call("LREM", KEYS[2], 0, jobId)', - ' redis.call("LPUSH", KEYS[3], jobId)', - ' redis.call("PUBLISH", KEYS[4], jobId)', - ' redis.call("HSET", ARGV[1] .. jobId, "delay", 0)', - ' local nextTimestamp = redis.call("ZRANGE", KEYS[1], 0, 0, "WITHSCORES")[2]', - ' if(nextTimestamp ~= nil) then', - ' nextTimestamp = nextTimestamp / 0x1000', - ' redis.call("PUBLISH", KEYS[1], nextTimestamp)', - ' end', - ' return nextTimestamp', - ' end', - ' return score', - 'end'].join('\n'); + var lua = loadScriptIfNeeded(queue, 'updateDelaySet'); var keys = _.map([ 'delayed', @@ -417,7 +445,7 @@ var scripts = { var args = [ queue.client, 'updateDelaySet', - script, + lua, keys.length, keys[0], keys[1], @@ -447,49 +475,19 @@ var scripts = { * timing window in which it must occur. */ moveUnlockedJobsToWait: function(queue){ - var script = [ - 'local MAX_STALLED_JOB_COUNT = tonumber(ARGV[1])', - 'local activeJobs = redis.call("LRANGE", KEYS[1], 0, -1)', - 'local stalled = {}', - 'local failed = {}', - 'for _, job in ipairs(activeJobs) do', - ' local jobKey = ARGV[2] .. job', - ' if(redis.call("EXISTS", jobKey .. ":lock") == 0) then', - // Remove from the active queue. - ' redis.call("LREM", KEYS[1], 1, job)', - ' local lockAcquired = redis.call("HGET", jobKey, "lockAcquired")', - ' if(lockAcquired) then', - // If it was previously locked then we consider it 'stalled' (Case A above). If this job - // has been stalled too many times, such as if it crashes the worker, then fail it. - ' local stalledCount = redis.call("HINCRBY", jobKey, "stalledCounter", 1)', - ' if(stalledCount > MAX_STALLED_JOB_COUNT) then', - ' redis.call("SADD", KEYS[3], job)', - ' redis.call("HSET", jobKey, "failedReason", "job stalled more than allowable limit")', - ' table.insert(failed, job)', - ' else', - // Move the job back to the wait queue, to immediately be picked up by a waiting worker. - ' redis.call("RPUSH", KEYS[2], job)', - ' table.insert(stalled, job)', - ' end', - ' else', - // Move the job back to the wait queue, to immediately be picked up by a waiting worker. - ' redis.call("RPUSH", KEYS[2], job)', - ' end', - ' end', - 'end', - 'return {failed, stalled}' - ].join('\n'); - + var lua = loadScriptIfNeeded(queue, 'moveUnlockedJobsToWait'); + var args = [ queue.client, 'moveUnlockedJobsToWait', - script, + lua, 3, queue.toKey('active'), queue.toKey('wait'), queue.toKey('failed'), queue.MAX_STALLED_JOB_COUNT, - queue.toKey('') + queue.toKey(''), + Date.now() ]; return execScript.apply(scripts, args); @@ -503,12 +501,6 @@ var scripts = { limit = limit || 0; switch(set) { - case 'completed': - case 'failed': - command = 'local jobs = redis.call("SMEMBERS", KEYS[1])'; - removeCommand = 'redis.call("SREM", KEYS[1], job)'; - hash = 'cleanSet'; - break; case 'wait': case 'active': case 'paused': @@ -517,6 +509,8 @@ var scripts = { hash = 'cleanList'; break; case 'delayed': + case 'completed': + case 'failed': command = 'local jobs = redis.call("ZRANGE", KEYS[1], 0, -1)'; removeCommand = 'redis.call("ZREM", KEYS[1], job)'; hash = 'cleanOSet'; @@ -589,7 +583,7 @@ var scripts = { var script = [ 'if (redis.call("EXISTS", KEYS[1]) == 1) then', ' if (redis.call("EXISTS", KEYS[2]) == 0) then', - ' if (redis.call("SREM", KEYS[3], ARGV[1]) == 1) then', + ' if (redis.call("ZREM", KEYS[3], ARGV[1]) == 1) then', ' redis.call("' + push + '", KEYS[4], ARGV[1])', ' redis.call("PUBLISH", KEYS[5], ARGV[1])', ' return 1', @@ -668,47 +662,32 @@ var emptyScript = [ ] - // this lua script takes three keys and two arguments - // keys: - // - the expanded key for the active set - // - the expanded key for the destination set - // - the expanded key for the job - // - // arguments: - // - json serialized context which is: - // - delayedTimestamp when the destination set is 'delayed' - // - stacktrace when the destination set is 'failed' - // - returnvalue of the handler when the destination set is 'completed' - // - the id of the job - // - // it checks whether KEYS[2] the destination set ends with 'delayed', 'completed' - // or 'failed'. And then adds the context to the jobhash and adds the job to - // the destination set. Finally it removes the job from the active list. - // - // it returns either 0 for success or -1 for failure. -var moveToSetScript = [ - 'if redis.call("EXISTS", KEYS[3]) == 1 then', - ' if string.find(KEYS[2], "delayed$") ~= nil then', - ' local score = tonumber(ARGV[1])', - ' if score ~= 0 then', - ' redis.call("ZADD", KEYS[2], score, ARGV[2])', - ' redis.call("PUBLISH", KEYS[2], (score / 0x1000))', - ' else', - ' redis.call("SADD", KEYS[2], ARGV[2])', - ' end', - ' elseif string.find(KEYS[2], "completed$") ~= nil then', - ' redis.call("HSET", KEYS[3], "returnvalue", ARGV[1])', - ' redis.call("SADD", KEYS[2], ARGV[2])', - ' elseif string.find(KEYS[2], "failed$") ~= nil then', - ' redis.call("SADD", KEYS[2], ARGV[2])', - ' else', - ' return -1', - ' end', - ' redis.call("LREM", KEYS[1], 0, ARGV[2])', - ' return 0', - 'else', - ' return -1', - 'end' - ].join('\n'); - module.exports = scripts; + +function wrapMove(job, move){ + var returnLockOrErrorCode = function(lock) { + return lock ? move() : -2; + }; + var throwUnexpectedErrors = function(err) { + if (!(err instanceof Redlock.LockError)) { + throw err; + } + }; + + return job.takeLock(!!job.lock) + .then(returnLockOrErrorCode, throwUnexpectedErrors) + .then(function(result){ + switch (result){ + case -1: + if(src){ + throw new Error('Missing Job ' + job.jobId + ' when trying to move from ' + src + ' to ' + target); + } else { + throw new Error('Missing Job ' + job.jobId + ' when trying to remove it from ' + src); + } + case -2: + throw new Error('Cannot get lock for job ' + job.jobId + ' when trying to move from ' + src); + default: + return job.releaseLock(); + } + }); +} diff --git a/lib/scripts/index.js b/lib/scripts/index.js new file mode 100644 index 000000000..f6c86a685 --- /dev/null +++ b/lib/scripts/index.js @@ -0,0 +1,36 @@ +/** + * Load redis lua scripts. + * The name of the script must have the following format: + * + * cmdName-numKeys.lua + * + * cmdName must be in camel case format. + * + * For example: + * moveToFinish-3.lua + * + */ + +var fs = require('fs'); +var path = require('path'); + +// +// for some very strange reason, defining scripts with this code results in this error +// when executing the scripts: ERR value is not an integer or out of range +// +module.exports = function(client){ + loadScripts(client, __dirname); +} + +function loadScripts(client, dir) { + fs.readdirSync(dir).filter(function (file) { + return path.extname(file) === '.lua'; + }).forEach(function (file) { + var longName = path.basename(file, '.lua'); + var name = longName.split('-')[0]; + var numberOfKeys = longName.split('-')[1]; + + var lua = fs.readFileSync(path.join(dir, file)).toString(); + client.defineCommand(name, { numberOfKeys: numberOfKeys, lua: lua }); + }); +}; diff --git a/lib/scripts/moveToFinished.lua b/lib/scripts/moveToFinished.lua new file mode 100644 index 000000000..3f24e72ec --- /dev/null +++ b/lib/scripts/moveToFinished.lua @@ -0,0 +1,41 @@ +--[[ + Move job from active to a finished status (completed o failed) + A job can only be moved to completed if it was active. + + Input: + KEYS[1] active key + KEYS[2] completed/failed key + KEYS[3] jobId key + + ARGV[1] jobId + ARGV[2] timestamp + ARGV[3] msg property + ARGV[4] return value / failed reason + ARGV[5] shouldRemove + ARGV[6] event channel + ARGV[7] event data (? maybe just send jobid). + + Output: + 0 OK + 1 Missing key. + + Events: + 'completed' +]] +if redis.call("EXISTS", KEYS[3]) == 1 then -- // Make sure job exists + redis.call("LREM", KEYS[1], -1, ARGV[1]) + + -- Remove job? + if ARGV[5] == "1" then + redis.call("DEL", KEYS[3]) + else + redis.call("ZADD", KEYS[2], ARGV[2], ARGV[1]) + redis.call("HSET", KEYS[3], ARGV[3], ARGV[4]) -- "returnvalue" / "failedReason" + end + + -- TODO PUBLISH EVENT, this is the most optimal way. + --redis.call("PUBLISH", ...) + return 0 +else + return -1 +end diff --git a/lib/scripts/moveToSet.lua b/lib/scripts/moveToSet.lua new file mode 100644 index 000000000..6b962843a --- /dev/null +++ b/lib/scripts/moveToSet.lua @@ -0,0 +1,42 @@ +--[[this lua script takes three keys and two arguments + // keys: + // - the expanded key for the active set + // - the expanded key for the destination set + // - the expanded key for the job + // + // arguments: + // ARGV[1] json serialized context which is: + // - delayedTimestamp when the destination set is 'delayed' + // - stacktrace when the destination set is 'failed' + // - returnvalue of the handler when the destination set is 'completed' + // ARGV[2] the id of the job + // ARGV[3] timestamp + // + // it checks whether KEYS[2] the destination set ends with 'delayed', 'completed' + // or 'failed'. And then adds the context to the jobhash and adds the job to + // the destination set. Finally it removes the job from the active list. + // + // it returns either 0 for success or -1 for failure. +]] +if redis.call("EXISTS", KEYS[3]) == 1 then + if string.find(KEYS[2], "delayed$") ~= nil then + local score = tonumber(ARGV[1]) + if score ~= 0 then + redis.call("ZADD", KEYS[2], score, ARGV[2]) + redis.call("PUBLISH", KEYS[2], (score / 0x1000)) + else + redis.call("ZADD", KEYS[2], ARGV[3], ARGV[2]) + end + elseif string.find(KEYS[2], "completed$") ~= nil then + redis.call("HSET", KEYS[3], "returnvalue", ARGV[1]) + redis.call("ZADD", KEYS[2], ARGV[3], ARGV[2]) + elseif string.find(KEYS[2], "failed$") ~= nil then + redis.call("ZADD", KEYS[2], ARGV[3], ARGV[2]) + else + return -1 + end + redis.call("LREM", KEYS[1], 0, ARGV[2]) + return 0 + else + return -1 +end diff --git a/lib/scripts/moveUnlockedJobsToWait.lua b/lib/scripts/moveUnlockedJobsToWait.lua new file mode 100644 index 000000000..535ca8ab3 --- /dev/null +++ b/lib/scripts/moveUnlockedJobsToWait.lua @@ -0,0 +1,56 @@ +--[[ + Looks for unlocked jobs in the active queue. There are two circumstances in which a job + would be in 'active' but NOT have a job lock: + + Case A) The job was being worked on, but the worker process died and it failed to renew the lock. + We call these jobs 'stalled'. This is the most common case. We resolve these by moving them + back to wait to be re-processed. To prevent jobs from cycling endlessly between active and wait, + (e.g. if the job handler keeps crashing), we limit the number stalled job recoveries to MAX_STALLED_JOB_COUNT. + + Case B) The job was just moved to 'active' from 'wait' and the worker that moved it hasn't gotten + a lock yet, or died immediately before getting the lock (note that due to Redis limitations, the + worker can't move the job and get the lock atomically - https://github.com/OptimalBits/bull/issues/258). + For this case we also move the job back to 'wait' for reprocessing, but don't consider it 'stalled' + since the job had never been started. This case is much rarer than Case A due to the very small + timing window in which it must occur. + + Input: + KEYS[1] 'active', + KEYS[2] 'wait', + KEYS[3] 'failed', + + ARGV[1] Max stalled job count + ARGV[2] queue.toKey('') + ARGV[3] timestamp +]] +local MAX_STALLED_JOB_COUNT = tonumber(ARGV[1]) +local activeJobs = redis.call("LRANGE", KEYS[1], 0, -1) +local stalled = {} +local failed = {} +for _, job in ipairs(activeJobs) do + + local jobKey = ARGV[2] .. job + if(redis.call("EXISTS", jobKey .. ":lock") == 0) then + -- Remove from the active queue. + redis.call("LREM", KEYS[1], 1, job) + local lockAcquired = redis.call("HGET", jobKey, "lockAcquired") + if(lockAcquired) then + -- If it was previously locked then we consider it 'stalled' (Case A above). If this job + -- has been stalled too many times, such as if it crashes the worker, then fail it. + local stalledCount = redis.call("HINCRBY", jobKey, "stalledCounter", 1) + if(stalledCount > MAX_STALLED_JOB_COUNT) then + redis.call("ZADD", KEYS[3], ARGV[3], job) + redis.call("HSET", jobKey, "failedReason", "job stalled more than allowable limit") + table.insert(failed, job) + else + -- Move the job back to the wait queue, to immediately be picked up by a waiting worker. + redis.call("RPUSH", KEYS[2], job) + table.insert(stalled, job) + end + else + -- Move the job back to the wait queue, to immediately be picked up by a waiting worker. + redis.call("RPUSH", KEYS[2], job) + end + end +end +return {failed, stalled} diff --git a/lib/scripts/removeJob.lua b/lib/scripts/removeJob.lua new file mode 100644 index 000000000..f4ecf9bbd --- /dev/null +++ b/lib/scripts/removeJob.lua @@ -0,0 +1,26 @@ +--[[ + Move job from active to a finished status (completed o failed) + A job can only be moved to completed if it was active. + + Input: + KEYS[1] 'active', + KEYS[2] 'wait', + KEYS[3] 'delayed', + KEYS[4] 'paused', + KEYS[5] 'completed', + KEYS[6] 'failed', + KEYS[7] jobId + + ARGV[1] jobId + + Events: + 'removed' +]] + +redis.call("LREM", KEYS[1], 0, ARGV[1]) +redis.call("LREM", KEYS[2], 0, ARGV[1]) +redis.call("ZREM", KEYS[3], ARGV[1]) +redis.call("LREM", KEYS[4], 0, ARGV[1]) +redis.call("ZREM", KEYS[5], ARGV[1]) +redis.call("ZREM", KEYS[6], ARGV[1]) +redis.call("DEL", KEYS[7]) diff --git a/lib/scripts/updateDelaySet.lua b/lib/scripts/updateDelaySet.lua new file mode 100644 index 000000000..a92c78af3 --- /dev/null +++ b/lib/scripts/updateDelaySet.lua @@ -0,0 +1,35 @@ +--[[ + Updates the delay set + + Input: + KEYS[1] 'delayed' + KEYS[2] 'active' + KEYS[3] 'wait' + KEYS[4] 'jobs' event channel + + ARGV[1] queue.toKey('') + ARGV[2] delayed timestamp + + Events: + 'removed' +]] +local RESULT = redis.call("ZRANGE", KEYS[1], 0, 0, "WITHSCORES") +local jobId = RESULT[1] +local score = RESULT[2] +if (score ~= nil) then + score = score / 0x1000 + if (score <= tonumber(ARGV[2])) then + redis.call("ZREM", KEYS[1], jobId) + redis.call("LREM", KEYS[2], 0, jobId) + redis.call("LPUSH", KEYS[3], jobId) + redis.call("PUBLISH", KEYS[4], jobId) + redis.call("HSET", ARGV[1] .. jobId, "delay", 0) + local nextTimestamp = redis.call("ZRANGE", KEYS[1], 0, 0, "WITHSCORES")[2] + if(nextTimestamp ~= nil) then + nextTimestamp = nextTimestamp / 0x1000 + redis.call("PUBLISH", KEYS[1], nextTimestamp) + end + return nextTimestamp + end + return score +end diff --git a/test/test_job.js b/test/test_job.js index edc5b884d..8019122b7 100644 --- a/test/test_job.js +++ b/test/test_job.js @@ -399,7 +399,7 @@ describe('Job', function(){ return job.getState(); }).then(function(state) { expect(state).to.be('completed'); - return client.srem(queue.toKey('completed'), job.jobId); + return client.zrem(queue.toKey('completed'), job.jobId); }).then(function(){ return job.moveToDelayed(Date.now() + 10000); }).then(function (){ @@ -419,7 +419,7 @@ describe('Job', function(){ return job.getState(); }).then(function(state) { expect(state).to.be('failed'); - return client.srem(queue.toKey('failed'), job.jobId); + return client.zrem(queue.toKey('failed'), job.jobId); }).then(function(res) { expect(res).to.be(1); return job.getState(); diff --git a/test/test_queue.js b/test/test_queue.js index faa4d3673..6ad36f920 100644 --- a/test/test_queue.js +++ b/test/test_queue.js @@ -862,7 +862,7 @@ describe('Queue', function () { expect(job.data.foo).to.be.equal('bar'); jobDone(); var client = new redis(); - client.srem(queue2.toKey('completed'), 1); + client.zrem(queue2.toKey('completed'), 1); client.lpush(queue2.toKey('active'), 1); }); @@ -2003,7 +2003,7 @@ describe('Queue', function () { }); queue.on('completed', _.after(3, function () { - queue.getJobs('completed', 'SET').then(function (jobs) { + queue.getJobs('completed', 'ZSET').then(function (jobs) { expect(jobs).to.be.an(Array); expect(jobs).to.have.length(3); done(); @@ -2021,7 +2021,7 @@ describe('Queue', function () { }); queue.on('failed', _.after(3, function () { - queue.getJobs('failed', 'SET').then(function (jobs) { + queue.getJobs('failed', 'ZSET').then(function (jobs) { expect(jobs).to.be.an(Array); expect(jobs).to.have.length(3); done(); @@ -2039,7 +2039,7 @@ describe('Queue', function () { }); queue.on('completed', _.after(3, function () { - queue.getJobs('completed', 'SET', 1, 2).then(function (jobs) { + queue.getJobs('completed', 'ZSET', 1, 2).then(function (jobs) { expect(jobs).to.be.an(Array); expect(jobs).to.have.length(2); expect(jobs[0].data.foo).to.be.equal(2); @@ -2059,7 +2059,7 @@ describe('Queue', function () { }); queue.on('completed', _.after(3, function () { - queue.getJobs('completed', 'SET', -3, -1).then(function (jobs) { + queue.getJobs('completed', 'ZSET', -3, -1).then(function (jobs) { expect(jobs).to.be.an(Array); expect(jobs).to.have.length(3); expect(jobs[0].data.foo).to.be.equal(1); @@ -2080,7 +2080,7 @@ describe('Queue', function () { }); queue.on('completed', _.after(3, function () { - queue.getJobs('completed', 'SET', -300, 99999).then(function (jobs) { + queue.getJobs('completed', 'ZSET', -300, 99999).then(function (jobs) { expect(jobs).to.be.an(Array); expect(jobs).to.have.length(3); expect(jobs[0].data.foo).to.be.equal(1); @@ -2145,14 +2145,13 @@ describe('Queue', function () { queue.process(function (job, jobDone) { jobDone(); }); - Promise.delay(100).then(function () { - return queue.clean(0); - }).then(function (jobs) { - expect(jobs.length).to.be(2); - done(); - }, function (err) { - done(err); - }); + + queue.on('completed', _.after(2, function(){ + queue.clean(0).then(function (jobs) { + expect(jobs.length).to.be(2); + done(); + }, done); + })); }); it('should only remove a job outside of the grace period', function (done) { From 73d7b47f89af4765a07df1979074efa90951fa64 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sat, 8 Apr 2017 00:40:49 +0200 Subject: [PATCH 02/17] refactored addJob into an external lua script --- lib/job.js | 104 ++++++++++----------------------- lib/scripts.js | 129 ++++++++--------------------------------- lib/scripts/addJob.lua | 99 +++++++++++++++++++++++++++++++ test/test_job.js | 2 +- test/test_queue.js | 3 +- 5 files changed, 158 insertions(+), 179 deletions(-) create mode 100644 lib/scripts/addJob.lua diff --git a/lib/job.js b/lib/job.js index 0a3ad6f0f..d06cc1c7b 100644 --- a/lib/job.js +++ b/lib/job.js @@ -14,6 +14,7 @@ interface JobOptions } */ + // queue: Queue, data: {}, opts: JobOptions var Job = function(queue, name, data, opts){ if(typeof name !== 'string'){ @@ -22,20 +23,15 @@ var Job = function(queue, name, data, opts){ name = '__default__'; } - opts = opts || {}; + this.opts = _.extend({}, opts); + this.opts.attempts = parseInt(this.opts.attempts || 1); this.name = name; this.queue = queue; this.data = data; - this.opts = opts; this._progress = 0; - this.delay = opts.delay || 0; - this.timestamp = opts.timestamp || Date.now(); + this.delay = this.opts.delay || 0; + this.timestamp = this.opts.timestamp || Date.now(); this.stacktrace = []; - if(this.opts.attempts > 1){ - this.attempts = opts.attempts; - }else{ - this.attempts = 1; - } this.returnvalue = null; this.attemptsMade = 0; }; @@ -78,22 +74,6 @@ Job.fromId = function(queue, jobId){ }); }; -Job.prototype.toData = function(){ - return { - name: this.name, - data: JSON.stringify(this.data || {}), - opts: JSON.stringify(this.opts || {}), - progress: this._progress, - delay: this.delay, - timestamp: this.timestamp, - attempts: this.attempts, - attemptsMade: this.attemptsMade, - failedReason: this.failedReason, - stacktrace: JSON.stringify(this.stacktrace || null), - returnvalue: JSON.stringify(this.returnvalue || null) - }; -}; - Job.prototype.progress = function(progress){ if(progress){ var _this = this; @@ -106,9 +86,6 @@ Job.prototype.progress = function(progress){ } }; -// -// toData and fromData should be deprecated. -// Job.prototype.toJSON = function(){ var opts = _.extend({}, this.opts || {}); opts.jobId = this.jobId; @@ -118,9 +95,8 @@ Job.prototype.toJSON = function(){ data: this.data || {}, opts: opts, progress: this._progress, - delay: this.delay, + delay: this.delay, // Move to opts timestamp: this.timestamp, - attempts: this.attempts, attemptsMade: this.attemptsMade, failedReason: this.failedReason, stacktrace: this.stacktrace || null, @@ -128,6 +104,16 @@ Job.prototype.toJSON = function(){ }; }; +Job.prototype.toData = function(){ + var json = this.toJSON(); + json.data = JSON.stringify(json.data); + json.opts = JSON.stringify(json.opts); + json.stacktrace = JSON.stringify(json.stacktrace); + json.returnvalue = JSON.stringify(json.returnvalue); + + return json; +}; + /** Return a unique key representing a lock for this Job */ @@ -202,7 +188,7 @@ Job.prototype.moveToFailed = function(err, noReleaseLock){ var promise; return this._saveAttempt(err).then(function() { // Check if an automatic retry should be performed - if(_this.attemptsMade < _this.attempts && !_this._discarded){ + if(_this.attemptsMade < _this.opts.attempts && !_this._discarded){ // Check if backoff is needed var backoff = _this._getBackOff(); if(backoff){ @@ -504,11 +490,8 @@ Job.prototype._retryAtOnce = function(){ }; Job.prototype._saveAttempt = function(err){ - if(isNaN(this.attemptsMade)){ - this.attemptsMade = 1; - }else{ - this.attemptsMade++; - } + this.attemptsMade++; + var params = { attemptsMade: this.attemptsMade }; @@ -520,52 +503,26 @@ Job.prototype._saveAttempt = function(err){ return this.queue.client.hmset(this.queue.toKey(this.jobId), params); }; -/** -*/ -Job.fromData = function(queue, jobId, data){ - var job = new Job(queue, data.name || Job.DEFAULT_JOB_NAME, JSON.parse(data.data), JSON.parse(data.opts)); +Job.fromData = function(queue, jobId, raw){ + raw = _.extend({}, raw); + raw.data = JSON.parse(raw.data); + raw.opts = JSON.parse(raw.opts); + + var job = Job.fromJSON(queue, raw); job.jobId = jobId; - job._progress = parseInt(data.progress); - job.delay = parseInt(data.delay); - job.timestamp = parseInt(data.timestamp); - - job.failedReason = data.failedReason; - job.attempts = parseInt(data.attempts); - if(isNaN(job.attempts)) { - job.attempts = 1; // Default to 1 try for legacy jobs - } - job.attemptsMade = parseInt(data.attemptsMade); - var _traces; - try{ - _traces = JSON.parse(data.stacktrace); - if(!(_traces instanceof Array)){ - _traces = []; - } - }catch (err){ - _traces = []; - } - job.stacktrace = _traces; - try{ - job.returnvalue = JSON.parse(data.returnvalue); - }catch (e){ - //swallow exception because the returnvalue got corrupted somehow. - debuglog('corrupted returnvalue: ' + data.returnvalue, e); - } return job; }; Job.fromJSON = function(queue, json){ var job = new Job(queue, json.name || Job.DEFAULT_JOB_NAME, json.data, json.opts); - job.jobId = json.opts.jobId; - job._progress = parseInt(json.progress); + + job._progress = parseInt(json.progress || 0); job.delay = parseInt(json.delay); job.timestamp = parseInt(json.timestamp); - job.attempts = parseInt(json.attempts); - if(isNaN(job.attempts)) { - job.attempts = 1; // Default to 1 try for legacy jobs - } - job.attemptsMade = parseInt(json.attemptsMade); + + job.failedReason = json.failedReason; + job.attemptsMade = parseInt(json.attemptsMade || 0); var _traces; try{ _traces = JSON.parse(json.stacktrace); @@ -583,6 +540,7 @@ Job.fromJSON = function(queue, json){ //swallow exception because the returnvalue got corrupted somehow. debuglog('corrupted returnvalue: ' + json.returnvalue, e); } + return job; } diff --git a/lib/scripts.js b/lib/scripts.js index 61406fc34..00bb08c3a 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -29,8 +29,8 @@ function isCommandDefined(client, hash){ var path = require('path'); var fs = require('fs'); -function loadScriptIfNeeded(queue, name){ - if(!queue.client[name]){ +function loadScriptIfNeeded(client, name){ + if(!client[name]){ return fs.readFileSync(path.join(__dirname, 'scripts/' + name + '.lua')).toString(); } } @@ -59,117 +59,38 @@ var scripts = { }); }, addJob: function(client, toKey, job, opts){ - var delayed; - var scriptName; - - opts = opts || {}; - opts.lifo = !!(opts.lifo); - - var delayTimestamp = job.timestamp + job.delay; - if(job.delay && delayTimestamp > Date.now()){ - delayed = true; - scriptName = 'addJob:delayed'; - } else { - scriptName = 'addJob'+(opts.lifo?':lifo':'') + (opts.priority?':priority':''); - } - - /* - if(isCommandDefined(client, scriptName)){ - return client[scriptName].apply(client, args); - }; - */ - - var jobArgs = _.flatten(_.toPairs(job)); + var queue = job.queue; + var lua = loadScriptIfNeeded(client, 'addJob'); var keys = _.map(['wait', 'paused', 'meta-paused', 'jobs', 'id', 'delayed', 'priority'], function(name){ return toKey(name); }); - var baseKey = toKey(''); - - var argvs = _.map(jobArgs, function(arg, index){ - return ', ARGV['+(index+4)+']'; - }) - - var script = [ - 'local jobCounter = redis.call("INCR", KEYS[5])', - 'local jobId', - 'if ARGV[2] == "" then jobId = jobCounter else jobId = ARGV[2] end', - 'local jobIdKey = ARGV[1] .. jobId', - 'if redis.call("EXISTS", jobIdKey) == 1 then return jobId end', - 'redis.call("HMSET", jobIdKey' + argvs.join('') + ')', - ]; - - var delayTimestamp = job.timestamp + job.delay; - if(delayed){ - script.push.apply(script, [ - ' local timestamp = tonumber(ARGV[' + (argvs.length + 4) + ']) * 0x1000 + bit.band(jobCounter, 0xfff)', - ' redis.call("ZADD", KEYS[6], timestamp, jobId)', - ' redis.call("PUBLISH", KEYS[6], (timestamp / 0x1000))', - ' return jobId', - ]); - }else{ - var push, pushPaused; - var add = _.template('redis.call("<%= direction %>", <%= waitQueue %>, jobId)'); - - if(opts.lifo){ - push = add({direction: 'RPUSH', waitQueue: 'KEYS[1]'}); - pushPaused = add({direction: 'RPUSH', waitQueue: 'KEYS[2]'}); - }else if(opts.priority){ - script.push.apply(script, [ - ' redis.call("ZADD", KEYS[7], ARGV[3], jobId)', - ' local count = redis.call("ZCOUNT", KEYS[7], 0, ARGV[3])', - ]); - - var priorityAdd = _.template([ - ' local len = redis.call("LLEN", <%= waitQueue %>)', - ' local id = redis.call("LINDEX", <%= waitQueue %>, len - (count-1))', - ' if id then', - ' redis.call("LINSERT", <%= waitQueue %>, "BEFORE", id, jobId)', - ' else', - ' redis.call("RPUSH", <%= waitQueue %>, jobId)', - ' end', - ].join('\n')); - - push = priorityAdd({waitQueue: 'KEYS[1]'}); - pushPaused = priorityAdd({waitQueue: 'KEYS[2]'}); - }else{ - push = add({direction: 'LPUSH', waitQueue: 'KEYS[1]'}); - pushPaused = add({direction: 'LPUSH', waitQueue: 'KEYS[2]'}); - } - - // - // Whe check for the meta-paused key to decide if we are paused or not - // (since an empty list and !EXISTS are not really the same) - script.push.apply(script, [ - 'if redis.call("EXISTS", KEYS[3]) ~= 1 then', - push, - 'else', - pushPaused, - 'end', - 'redis.call("PUBLISH", KEYS[4], jobId)', - 'return jobId .. ""' - ]); - } var args = [ client, - scriptName, - script.join('\n'), + 'addJob', + lua, keys.length - ]; + ] args.push.apply(args, keys); - args.push(baseKey); - args.push(opts.customJobId || ''); - args.push(opts.priority); - args.push.apply(args, jobArgs); - args.push(delayTimestamp); + args.push.apply(args, [ + toKey(''), + _.isUndefined(opts.customJobId) ? "" : opts.customJobId, + job.name, + job.data, + job.opts, + job.timestamp, + job.delay, + job.delay ? job.timestamp + job.delay : "0", + opts.priority || 0, + opts.lifo ? "LIFO" : "FIFO" + ]); return execScript.apply(scripts, args); }, - // TODO: perfect this function so that it can be used instead - // of all the specialized functions moveToComplete, etc. + // TODO: DEPRECATE. move: function(job, src, target){ // TODO: Depending on the source we should use LREM or ZREM. // TODO: Depending on the target we should use LPUSH, SADD, etc. @@ -244,7 +165,7 @@ var scripts = { moveToFinished: function(job, val, propVal, shouldRemove, target, shouldLock){ var queue = job.queue; - var lua = loadScriptIfNeeded(queue, 'moveToFinished'); + var lua = loadScriptIfNeeded(queue.client, 'moveToFinished'); var keys = _.map([ 'active', @@ -287,7 +208,7 @@ var scripts = { }, moveToSet: function(queue, set, jobId, context){ - var lua = loadScriptIfNeeded(queue, 'moveToSet'); + var lua = loadScriptIfNeeded(queue.client, 'moveToSet'); // // Bake in the job id first 12 bits into the timestamp @@ -335,7 +256,7 @@ var scripts = { }, remove: function(queue, jobId){ - var lua = loadScriptIfNeeded(queue, 'removeJob'); + var lua = loadScriptIfNeeded(queue.client, 'removeJob'); var keys = _.map([ 'active', @@ -432,7 +353,7 @@ var scripts = { top of the wait queue (so that it will be processed as soon as possible) */ updateDelaySet: function(queue, delayedTimestamp){ - var lua = loadScriptIfNeeded(queue, 'updateDelaySet'); + var lua = loadScriptIfNeeded(queue.client, 'updateDelaySet'); var keys = _.map([ 'delayed', @@ -475,7 +396,7 @@ var scripts = { * timing window in which it must occur. */ moveUnlockedJobsToWait: function(queue){ - var lua = loadScriptIfNeeded(queue, 'moveUnlockedJobsToWait'); + var lua = loadScriptIfNeeded(queue.client, 'moveUnlockedJobsToWait'); var args = [ queue.client, diff --git a/lib/scripts/addJob.lua b/lib/scripts/addJob.lua new file mode 100644 index 000000000..b7bf91f94 --- /dev/null +++ b/lib/scripts/addJob.lua @@ -0,0 +1,99 @@ +--[[ + Adds a job to the queue by doing the following: + - Increases the job counter if needed. + - Creates a new job key with the job data. + + - if delayed: + - computes timestamp. + - adds to delayed zset. + - Emits a global event 'delayed' if the job is delayed. + - if not delayed + - Adds the jobId to the wait/paused list in one of three ways: + - LIFO + - FIFO + - prioritized. + - Emits a global event 'waiting' if not paused. + + Input: + KEYS[1] 'wait', + KEYS[2] 'paused' + KEYS[3] 'meta-paused' + KEYS[4] 'jobs' + KEYS[5] 'id' + KEYS[6] 'delayed' + KEYS[7] 'priority' + + ARGV[1] key prefix, + ARGV[2] custom id (will not generate one automatically) + ARGV[3] name + ARGV[4] data (json stringified job data) + ARGV[5] opts (json stringified job opts) + ARGV[6] timestamp + ARGV[7] delay + ARGV[8] delayedTimestamp + ARGV[9] priority + ARGV[10] LIFO + + Events: + 'waiting' +]] +local jobCounter = redis.call("INCR", KEYS[5]) -- Why doing this allways ? +local jobId +if ARGV[2] == "" then + jobId = jobCounter +else + jobId = ARGV[2] +end + +local jobIdKey = ARGV[1] .. jobId +if redis.call("EXISTS", jobIdKey) == 1 then + return jobId .. "" -- convert to string +end + +-- Store the job. +redis.call("HMSET", jobIdKey, "name", ARGV[3], "data", ARGV[4], "opts", ARGV[5], "timestamp", ARGV[6], "delay", ARGV[7]) + +-- Check if job is delayed +if(ARGV[8] ~= "0") then + local timestamp = tonumber(ARGV[8]) * 0x1000 + bit.band(jobCounter, 0xfff) + redis.call("ZADD", KEYS[6], timestamp, jobId) + redis.call("PUBLISH", KEYS[6], (timestamp / 0x1000)) + return jobId .. "" -- convert to string +else + local direction + local target + + if ARGV[10] == "LIFO" then + direction = "RPUSH" + else + direction = "LPUSH" + end + + -- Whe check for the meta-paused key to decide if we are paused or not + -- (since an empty list and !EXISTS are not really the same) + if redis.call("EXISTS", KEYS[3]) ~= 1 then + target = KEYS[1] + else + target = KEYS[2] + end + + if ARGV[9] == "0" then + -- Standard add + redis.call(direction, target, jobId) + else + -- Priority add + redis.call("ZADD", KEYS[7], ARGV[9], jobId) + local count = redis.call("ZCOUNT", KEYS[7], 0, ARGV[9]) + + local len = redis.call("LLEN", target) + local id = redis.call("LINDEX", target, len - (count-1)) + if id then + redis.call("LINSERT", target, "BEFORE", id, jobId) + else + redis.call("RPUSH", target, jobId) + end + end + + redis.call("PUBLISH", KEYS[4], jobId) + return jobId .. "" -- convert to string +end diff --git a/test/test_job.js b/test/test_job.js index 8019122b7..d381040e4 100644 --- a/test/test_job.js +++ b/test/test_job.js @@ -77,7 +77,7 @@ describe('Job', function(){ queue.add({ foo: 'bar' }, { jobId: customJobId }); queue.on('completed', function(job) { - if (job.opts.jobId == customJobId) { + if (job.jobId == customJobId) { done(); } }); diff --git a/test/test_queue.js b/test/test_queue.js index 6ad36f920..2db6b4f84 100644 --- a/test/test_queue.js +++ b/test/test_queue.js @@ -1637,11 +1637,12 @@ describe('Queue', function () { var tries = 0; queue.process(function (job, jobDone) { + expect(job.attemptsMade).to.be(tries); tries++; if (job.attemptsMade < 2) { throw new Error('Not yet!'); } - expect(job.attemptsMade).to.be(tries - 1); + jobDone(); }); From c069b7ab1cd9aa2c17b853cebab924f57838745f Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Wed, 12 Apr 2017 19:20:22 +0200 Subject: [PATCH 03/17] Initial implementation of #480 --- CHANGELOG.md | 5 + lib/errors.js | 7 + lib/job.js | 136 +++-------- lib/queue.js | 326 ++++++++++--------------- lib/scripts.js | 224 ++++++++++++++--- lib/scripts/addJob.lua | 4 +- lib/scripts/extendLock.lua | 19 ++ lib/scripts/moveToActive.lua | 42 ++++ lib/scripts/moveToFinished.lua | 25 +- lib/scripts/moveUnlockedJobsToWait.lua | 32 ++- lib/scripts/pause.lua | 18 ++ lib/scripts/releaseLock.lua | 17 ++ lib/scripts/removeJob.lua | 28 ++- lib/scripts/retryJob.lua | 26 ++ lib/scripts/takeLock.lua | 17 ++ lib/scripts/updateDelaySet.lua | 4 +- test/test_connection.js | 22 +- test/test_job.js | 63 +++-- test/test_queue.js | 253 ++++++++++--------- test/utils.js | 1 - 20 files changed, 726 insertions(+), 543 deletions(-) create mode 100644 lib/errors.js create mode 100644 lib/scripts/extendLock.lua create mode 100644 lib/scripts/moveToActive.lua create mode 100644 lib/scripts/pause.lua create mode 100644 lib/scripts/releaseLock.lua create mode 100644 lib/scripts/retryJob.lua create mode 100644 lib/scripts/takeLock.lua diff --git a/CHANGELOG.md b/CHANGELOG.md index 84f3f9189..42533b1a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +v.3.0.0-alpha +============= + + + v.2.2.6 ======= diff --git a/lib/errors.js b/lib/errors.js new file mode 100644 index 000000000..222b8c48b --- /dev/null +++ b/lib/errors.js @@ -0,0 +1,7 @@ + + +module.exports.Messages = { + RETRY_JOB_NOT_EXIST: 'Couldn\'t retry job: The job doesn\'t exist', + RETRY_JOB_IS_LOCKED: 'Couldn\'t retry job: The job is locked', + RETRY_JOB_NOT_FAILED: 'Couldn\'t retry job: The job has been already retried or has not failed' +} diff --git a/lib/job.js b/lib/job.js index d06cc1c7b..32fbad788 100644 --- a/lib/job.js +++ b/lib/job.js @@ -5,6 +5,7 @@ var Promise = require('bluebird'); var _ = require('lodash'); var scripts = require('./scripts'); var debuglog = require('debuglog')('bull'); +var errors = require('./errors'); /** interface JobOptions @@ -53,7 +54,7 @@ Job.create = function(queue, name, data, opts){ var job = new Job(queue, name, data, opts); return addJob(queue, job).then(function(jobId){ - job.jobId = jobId; + job.id = jobId; queue.distEmit('waiting', job); debuglog('Job added', jobId); return job; @@ -67,7 +68,7 @@ Job.fromId = function(queue, jobId){ } return queue.client.hgetall(queue.toKey(jobId)).then(function(jobData){ if(!_.isEmpty(jobData)){ - return Job.fromData(queue, jobId, jobData); + return Job.fromData(queue, jobData); }else{ return null; } @@ -78,7 +79,7 @@ Job.prototype.progress = function(progress){ if(progress){ var _this = this; this._progress = progress; - return this.queue.client.hset(this.queue.toKey(this.jobId), 'progress', progress).then(function(){ + return this.queue.client.hset(this.queue.toKey(this.id), 'progress', progress).then(function(){ _this.queue.distEmit('progress', _this, progress); }); }else{ @@ -88,10 +89,9 @@ Job.prototype.progress = function(progress){ Job.prototype.toJSON = function(){ var opts = _.extend({}, this.opts || {}); - opts.jobId = this.jobId; + opts.jobId = this.id; return { name: this.name, - id: this.jobId, data: this.data || {}, opts: opts, progress: this._progress, @@ -118,19 +118,15 @@ Job.prototype.toData = function(){ Return a unique key representing a lock for this Job */ Job.prototype.lockKey = function(){ - return this.queue.toKey(this.jobId) + ':lock'; + return this.queue.toKey(this.id) + ':lock'; }; /** Takes a lock for this job so that no other queue worker can process it at the same time. */ -Job.prototype.takeLock = function(renew, ensureActive){ - var _this = this; - return scripts.takeLock(this.queue, this, renew, ensureActive).then(function(lock) { - if (lock){ - _this.lock = lock; - } +Job.prototype.takeLock = function(){ + return scripts.takeLock(this.queue, this).then(function(lock) { return lock || false; }); }; @@ -147,8 +143,10 @@ Job.prototype.renewLock = function(){ */ Job.prototype.releaseLock = function(){ var _this = this; - return scripts.releaseLock(this).then(function() { - _this.lock = null; + return scripts.releaseLock(this.queue, this.id).then(function(unlocked) { + if(unlocked != 1){ + throw Error('Could not release lock for job ' + _this.id); + } }); }; @@ -164,9 +162,9 @@ Job.prototype.delayIfNeeded = function(){ return Promise.resolve(false); }; -Job.prototype.moveToCompleted = function(returnValue){ +Job.prototype.moveToCompleted = function(returnValue, ignoreLock){ this.returnvalue = returnValue || 0; - return scripts.moveToCompleted(this, this.returnvalue, this.opts.removeOnComplete); + return scripts.moveToCompleted(this, this.returnvalue, this.opts.removeOnComplete, ignoreLock); }; Job.prototype.move = function(src, target, returnValue){ @@ -183,34 +181,25 @@ Job.prototype.discard = function(){ this._discarded = true; } -Job.prototype.moveToFailed = function(err, noReleaseLock){ +// This needs to be atomized in a LUA script. +Job.prototype.moveToFailed = function(err, ignoreLock){ var _this = this; - var promise; return this._saveAttempt(err).then(function() { // Check if an automatic retry should be performed if(_this.attemptsMade < _this.opts.attempts && !_this._discarded){ // Check if backoff is needed var backoff = _this._getBackOff(); if(backoff){ - // If so, move to delayed - promise = _this.moveToDelayed(Date.now() + backoff); + // If so, move to delayed (need to unlock job in this case!) + return _this.moveToDelayed(Date.now() + backoff); }else{ // If not, retry immediately - promise = _this._retryAtOnce(); + return scripts.retryJob(_this); } - } else if(_this.opts.removeOnFail){ - return _this.releaseLock().then(function(){ - return _this.remove(); - }); } else { // If not, move to failed - promise = scripts.moveToFailed(_this, err.message, _this.opts.removeOnFail); + return scripts.moveToFailed(_this, err.message, _this.opts.removeOnFail, ignoreLock); } - return promise.then(function(){ - if(!noReleaseLock){ - return _this.releaseLock(); - } - }); }); }; @@ -220,7 +209,7 @@ Job.prototype.moveToDelayed = function(timestamp){ Job.prototype.promote = function(){ var queue = this.queue; - var jobId = this.jobId; + var jobId = this.id; var script = [ 'if redis.call("ZREM", KEYS[1], ARGV[1]) == 1 then', @@ -261,11 +250,11 @@ Job.prototype.retry = function(){ if (result === 1) { queue.emit('waiting', _this); } else if (result === 0) { - throw new Error('Couldn\'t retry job: The job doesn\'t exist'); + throw new Error(errors.Messages.RETRY_JOB_NOT_EXIST); } else if (result === -1) { - throw new Error('Couldn\'t retry job: The job is locked'); + throw new Error(errors.Messages.RETRY_JOB_IS_LOCKED); } else if (result === -2) { - throw new Error('Couldn\'t retry job: The job has been already retried or has not failed'); + throw new Error(errors.Messages.RETRY_JOB_NOT_FAILED); } }); }; @@ -279,10 +268,7 @@ Job.prototype.isFailed = function(){ }; Job.prototype.isDelayed = function() { - return this.queue.client - .zrank(this.queue.toKey('delayed'), this.jobId).then(function(rank) { - return rank !== null; - }); + return this._isDone('delayed'); }; Job.prototype.isActive = function() { @@ -326,26 +312,16 @@ Job.prototype.getState = function() { }); }; -/** - Removes a job from the queue and from all the lists where it may be stored. -*/ Job.prototype.remove = function(){ var queue = this.queue; var job = this; - return job.takeLock().then(function(lock) { - if (!lock) { - throw new Error('Could not get lock for job: ' + job.jobId + '. Cannot remove job.'); + return scripts.remove(queue, job.id).then(function(removed) { + if(removed){ + queue.emit('removed', job); + }else{ + throw Error('Could not remove job ' + job.id); } - return scripts.remove(queue, job.jobId) - .then(function() { - queue.emit('removed', job); - }) - .finally(function () { - return job.releaseLock().catch(function(err){ - queue.emit('error', err); - }) - }); }); }; @@ -362,7 +338,7 @@ Job.prototype.finished = function(){ if(!completed){ return _this.isFailed().then(function(failed){ if(failed){ - return Job.fromId(_this.queue, _this.jobId, 'failedReason').then(function(data){ + return Job.fromId(_this.queue, _this.id, 'failedReason').then(function(data){ reject(Error(data.failedReason)); return true; }); @@ -379,7 +355,7 @@ Job.prototype.finished = function(){ if(!finished){ var interval; function onCompleted(job){ - if(String(job.jobId) === String(_this.jobId)){ + if(String(job.id) === String(_this.id)){ resolve(); removeListeners(); clearInterval(interval); @@ -387,7 +363,7 @@ Job.prototype.finished = function(){ } function onFailed(job, err){ - if(String(job.jobId) === String(_this.jobId)){ + if(String(job.id) === String(_this.id)){ reject(err); removeListeners(); clearInterval(interval); @@ -423,18 +399,18 @@ Job.prototype.finished = function(){ // ----------------------------------------------------------------------------- Job.prototype._isDone = function(list){ return this.queue.client - .zscore(this.queue.toKey(list), this.jobId).then(function(score){ + .zscore(this.queue.toKey(list), this.id).then(function(score){ return score !== null; }); }; Job.prototype._isInList = function(list) { - return scripts.isJobInList(this.queue.client, this.queue.toKey(list), this.jobId); + return scripts.isJobInList(this.queue.client, this.queue.toKey(list), this.id); }; Job.prototype._moveToSet = function(set, context){ var queue = this.queue; - var jobId = this.jobId; + var jobId = this.id; return scripts.moveToSet(queue, set, jobId, context); }; @@ -455,40 +431,6 @@ Job.prototype._getBackOff = function() { return backoff; }; -Job.prototype._retryAtOnce = function(){ - var queue = this.queue; - var jobId = this.jobId; - - var script = [ - 'if redis.call("EXISTS", KEYS[3]) == 1 then', - ' redis.call("LREM", KEYS[1], 0, ARGV[2])', - ' redis.call(ARGV[1], KEYS[2], ARGV[2])', - ' return 0', - 'else', - ' return -1', - 'end' - ].join('\n'); - - var keys = _.map(['active', 'wait', jobId], function(name){ - return queue.toKey(name); - }); - - var pushCmd = (this.opts.lifo ? 'R' : 'L') + 'PUSH'; - - return queue.client.eval( - script, - keys.length, - keys[0], - keys[1], - keys[2], - pushCmd, - jobId).then(function(result){ - if(result === -1){ - throw new Error('Missing Job ' + jobId + ' during retry'); - } - }); -}; - Job.prototype._saveAttempt = function(err){ this.attemptsMade++; @@ -500,16 +442,15 @@ Job.prototype._saveAttempt = function(err){ params.stacktrace = JSON.stringify(this.stacktrace); params.failedReason = err.message; - return this.queue.client.hmset(this.queue.toKey(this.jobId), params); + return this.queue.client.hmset(this.queue.toKey(this.id), params); }; -Job.fromData = function(queue, jobId, raw){ +Job.fromData = function(queue, raw){ raw = _.extend({}, raw); raw.data = JSON.parse(raw.data); raw.opts = JSON.parse(raw.opts); var job = Job.fromJSON(queue, raw); - job.jobId = jobId; return job; }; @@ -517,6 +458,7 @@ Job.fromData = function(queue, jobId, raw){ Job.fromJSON = function(queue, json){ var job = new Job(queue, json.name || Job.DEFAULT_JOB_NAME, json.data, json.opts); + job.id = json.id; job._progress = parseInt(json.progress || 0); job.delay = parseInt(json.delay); job.timestamp = parseInt(json.timestamp); diff --git a/lib/queue.js b/lib/queue.js index 852aa4370..02dd6e309 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -8,11 +8,14 @@ var assert = require('assert'); var url = require('url'); var Job = require('./job'); var scripts = require('./scripts'); +var errors = require('./errors'); + var TimerManager = require('./timer-manager'); var _ = require('lodash'); var Promise = require('bluebird'); var semver = require('semver'); var debuglog = require('debuglog')('bull'); +var uuid = require('uuid'); /** Gets or creates a new Queue with the given name. @@ -26,7 +29,7 @@ var debuglog = require('debuglog')('bull'); - failed (set) --> priorities -- >completed - / / + / | / job -> wait -> active | ^ \ v | -- > failed @@ -54,7 +57,6 @@ var STALLED_JOB_CHECK_INTERVAL = 5000; // 5 seconds is the renew time. // (moved back to 'wait'), before it is failed. var MAX_STALLED_JOB_COUNT = 1; -var CLIENT_CLOSE_TIMEOUT_MS = 5000; var POLLING_INTERVAL = 5000; var REDLOCK_DRIFT_FACTOR = 0.01; @@ -113,6 +115,7 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ this.name = name; this.keyPrefix = redisOptions.keyPrefix || 'bull'; + this.token = uuid(); // // We cannot use ioredis keyPrefix feature until we @@ -147,11 +150,6 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ }; _.extend(this.redlock, redisOptions.redlock || {}); - // - // Create blocking client (used to wait for jobs) - // - this.bclient = createClient('block'); - // // Create event subscriber client (receive messages from other instance of the queue) // @@ -159,7 +157,7 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ this.handlers = {}; this.delayTimer = null; - this.processing = 0; + this.processing = Promise.resolve(); this.retrieving = 0; this.LOCK_DURATION = LOCK_DURATION; @@ -168,7 +166,7 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ this.MAX_STALLED_JOB_COUNT = MAX_STALLED_JOB_COUNT; // bubble up Redis error events - [this.client, this.bclient, this.eclient].forEach(function (client) { + [this.client, this.eclient].forEach(function (client) { client.on('error', _this.emit.bind(_this, 'error')); }); @@ -178,18 +176,24 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ this.timers = new TimerManager(); // emit ready when redis connections ready - var initializers = [this.client, this.bclient, this.eclient].map(function (client) { + var initializers = [this.client, this.eclient].map(function (client) { return new Promise(function(resolve, reject) { client.once('ready', resolve); client.once('error', reject); }); }); + + var events = [ + 'delayed', + 'paused', + 'resumed', + 'added' + ] this._initializing = Promise.all(initializers).then(function(){ - return Promise.join( - _this.eclient.subscribe(_this.toKey('delayed')), - _this.eclient.subscribe(_this.toKey('paused')) - ); + return Promise.all(events.map(function(event){ + return _this.eclient.subscribe(_this.toKey(event)) + })) }).then(function(){ debuglog(name + ' queue ready'); _this.emit('ready'); @@ -197,6 +201,24 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ _this.emit('error', err, 'Error initializing queue'); }); + // + // Handle delay, pause and resume messages + // + this.eclient.on('message', function(channel, message){ + switch(channel){ + case _this.toKey('delayed'): + _this.updateDelayTimer(message); + break; + case _this.toKey('paused'): + case _this.toKey('resumed'): + _this.emit(message); + break; + case _this.toKey('added'): + _this.emit('added', message); + break; + } + }); + Disturbed.call(this, _this.client, _this.eclient); // @@ -209,7 +231,7 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ listenDistEvent('active'); // listenDistEvent('completed'); // listenDistEvent('failed'); // - listenDistEvent('cleaned'); + listenDistEvent('cleaned'); listenDistEvent('remove'); // function listenDistEvent(eventName){ @@ -226,19 +248,6 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ }, true); } - // - // Handle delay, pause and resume messages - // - var delayedKey = _this.toKey('delayed'); - var pausedKey = _this.toKey('paused'); - this.eclient.on('message', function(channel, message){ - if(channel === delayedKey){ - _this.updateDelayTimer(message); - }else if(channel === pausedKey){ - _this.emit(message); - } - }); - // // Init delay timestamp. // @@ -268,7 +277,6 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ // Bind these methods to avoid constant rebinding and/or creating closures // in processJobs etc. this.moveUnlockedJobsToWait = this.moveUnlockedJobsToWait.bind(this); - this.getNextJob = this.getNextJob.bind(this); this.processJobs = this.processJobs.bind(this); this.processJob = this.processJob.bind(this); this.getJobFromId = Job.fromId.bind(null, this); @@ -276,6 +284,8 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ util.inherits(Queue, Disturbed); +Queue.ErrorMessages = errors.Messages; + Queue.prototype.isReady = function(){ var _this = this; return this._initializing.then(function(){ @@ -284,11 +294,11 @@ Queue.prototype.isReady = function(){ } Queue.prototype.getJobMoveCount = function(){ - return this.bclient.commandQueue.length; + return this.client.commandQueue.length; }; Queue.prototype.whenCurrentMoveFinished = function(){ - var currentMove = this.bclient.commandQueue.peekFront() + var currentMove = this.client.commandQueue.peekFront() return currentMove && currentMove.command.promise || Promise.resolve(); }; /** @@ -321,26 +331,21 @@ Queue.prototype.once = function(){ }; Queue.prototype.disconnect = function(){ - var _this = this; - - function endClients(){ - var timeoutMsg = 'Timed out while waiting for redis clients to close'; + var clients = [this.client, this.eclient].filter(function(client){ + return client.status === 'ready'; + }); - return new Promise(function(resolve) { - _this.bclient.disconnect(); - _this.bclient.stream.once('close', resolve); - }).timeout(CLIENT_CLOSE_TIMEOUT_MS, timeoutMsg) - .catch(function(err){ - if(!(err instanceof Promise.TimeoutError)){ - throw err; - } + var ended = new Promise(function(resolve){ + var resolver = _.after(clients.length, resolve); + clients.forEach(function(client){ + client.once('end', resolver); }); - } - - return Promise.join( - _this.client.quit(), - _this.eclient.quit() - ).then(endClients, endClients); + }); + return Promise.all(clients.map(function(client){ + return client.quit(); + })).then(function(){ + return ended; + }); }; Queue.prototype.close = function( doNotWaitJobs ){ @@ -355,7 +360,6 @@ Queue.prototype.close = function( doNotWaitJobs ){ clearInterval(_this.guardianTimer); clearInterval(_this.moveUnlockedJobsToWaitInterval); _this.timers.clearAll(); - return _this.timers.whenIdle().then(function(){ return _this.pause(true, doNotWaitJobs); }).then(function(){ @@ -426,7 +430,10 @@ interface JobOptions @param opts: JobOptions Options for this job. */ Queue.prototype.add = function(name, data, opts){ - return Job.create(this, name, data, opts); + var _this = this; + return this.isReady().then(function(){ + return Job.create(_this, name, data, opts); + }); }; /** @@ -504,7 +511,6 @@ Queue.prototype.pause = function(isLocal, doNotWaitActive){ }; }); } - return !doNotWaitActive && this.whenCurrentJobsFinished(); }else{ return pauseResumeGlobal(this, true); @@ -556,7 +562,6 @@ Queue.prototype.run = function(concurrency){ var _this = this; return this.moveUnlockedJobsToWait().then(function(){ - while(concurrency--){ promises.push(new Promise(_this.processJobs)); } @@ -577,7 +582,6 @@ Queue.prototype.run = function(concurrency){ */ Queue.prototype.updateDelayTimer = function(newDelayedTimestamp){ var _this = this; - if(newDelayedTimestamp < _this.delayedTimestamp && newDelayedTimestamp < (MAX_TIMEOUT_MS + Date.now())){ clearTimeout(this.delayTimer); this.delayedTimestamp = newDelayedTimestamp; @@ -643,32 +647,36 @@ Queue.prototype.startMoveUnlockedJobsToWait = function() { } }; +// Idea to be able to know when a processing promise is finished, just store the promise + Queue.prototype.processJobs = function(resolve, reject){ var _this = this; var processJobs = this.processJobs.bind(this, resolve, reject); - - if(!this.closing){ - process.nextTick(function(){ - (_this.paused || Promise.resolve()) - .then(_this.getNextJob) - .then(_this.processJob) - .then(processJobs, function(err){ - _this.emit('error', err, 'Error processing job'); - processJobs(); - }).catch(reject); - }); - }else{ - resolve(this.closing); - } + process.nextTick(function(){ + if(!_this.closing){ + (_this.paused || Promise.resolve()).then(function(){ + return _this.processing = _this.getNextJob() + .then(_this.processJob) + .then(processJobs, function(err){ + _this.emit('error', err, 'Error processing job'); + processJobs(); + }); + }).catch(reject); // Not sure this catch is correct here. + }else{ + resolve(_this.closing); + } + }); }; Queue.prototype.processJob = function(job){ var _this = this; var lockRenewId; var timmerStopped = false; + if(!job){ return Promise.resolve(); } + // // TODO: // There are two cases to take into consideration regarding locks. @@ -678,20 +686,17 @@ Queue.prototype.processJob = function(job){ // jobs, so we can assume the job has been stalled and is already being processed // by another worker. See #308 // - var renew = false; - var lockRenewer = function(){ - return job.takeLock(renew, true).then(function(lock){ - if(lock && !timmerStopped){ - renew = true; - lockRenewId = _this.timers.set('lockRenewer', _this.LOCK_RENEW_TIME, lockRenewer); + var lockExtender = function(){ + _this.timers.set('lockExtender', _this.LOCK_RENEW_TIME, function(){ + if(!timmerStopped){ + scripts.extendLock(_this, job.id).then(function(lock){ + if(lock){ + lockExtender(); + } + }); } - // TODO: if we failed to re-acquire the lock while trying to renew, should we let the job - // handler know and cancel the timer? - return lock; - }, function(err){ - _this.emit('error', err, 'Error renewing lock'); }); - }; + } var timeoutMs = job.opts.timeout; @@ -700,24 +705,17 @@ Queue.prototype.processJob = function(job){ _this.timers.clear(lockRenewId); } - function handleCompleted(data){ + function handleCompleted(result){ try{ - JSON.stringify(data); + JSON.stringify(result); }catch(err){ return handleFailed(err); } stopTimer(); - if(_this.closed){ - return; - } - return job.moveToCompleted(data).finally(function(){ - // This substraction is duplicate in handleCompleted and handleFailed because it have to be made before throwing any - // event completed or failed in order to allow pause() to work correctly without getting stuck. - _this.processing--; - }).then(function(){ - return _this.distEmit('completed', job, data); + return job.moveToCompleted(result).then(function(){ + return _this.distEmit('completed', job, result); }); } @@ -727,63 +725,26 @@ Queue.prototype.processJob = function(job){ stopTimer(); // See https://github.com/OptimalBits/bull/pull/415#issuecomment-269744735 - job.takeLock(true /* renew */, false /* ensureActive */).then( function(/*lock*/) { - return job.moveToFailed(err).finally(function(){ - _this.processing--; - }).then(function(){ - return _this.distEmit('failed', job, error); - }); - }, function(err){ - _this.emit('error', err, 'failed to re-obtain lock before moving to failed, bailing'); + // job.takeLock(true /* renew */, false /* ensureActive */).then( function(/*lock*/) { + return job.moveToFailed(err).then(function(){ + return _this.distEmit('failed', job, error); }); } - return lockRenewer().then(function(locked){ - if(locked){ - var handler = _this.handlers[job.name]; - if(!handler){ - return handleFailed(Error('Missing process handler for job type ' + job.name)); - }else{ - var jobPromise = handler(job); - - if(timeoutMs){ - jobPromise = jobPromise.timeout(timeoutMs); - } - - _this.distEmit('active', job, jobPromise); + lockExtender() + var handler = _this.handlers[job.name]; + if(!handler){ + return handleFailed(Error('Missing process handler for job type ' + job.name)); + }else{ + var jobPromise = handler(job); - return jobPromise.then(handleCompleted, handleFailed); - } - }else{ - _this.processing--; - throw Error('Failed getting the lock') + if(timeoutMs){ + jobPromise = jobPromise.timeout(timeoutMs); } - }); -}; -/** - Returns a promise that resolves to the next job in queue. -*/ -Queue.prototype.getNextJob = function(opts){ - var _this = this; - if(!this.closing){ - this.retrieving++; - return this.moveJob('wait', 'active', opts) - .then(this.getJobFromId) - .tap(function(job) { - if (job) { - _this.processing++; - } else { - _this.emit('no-job-retrieved'); - } - }) - .finally(function(){ - _this.retrieving--; - }) - .catch(function(err) { - _this.emit('no-job-retrieved'); - throw err; - }); + _this.distEmit('active', job, jobPromise); + + return jobPromise.then(handleCompleted, handleFailed); } }; @@ -792,46 +753,30 @@ Queue.prototype.multi = function(){ }; /** - Atomically moves a job from one list to another. - - @method moveJob + Returns a promise that resolves to the next job in queue. */ -Queue.prototype.moveJob = function(src, dst, opts) { - var args = arguments; +Queue.prototype.getNextJob = function() { var _this = this; - var move; - if(opts && opts.block === false){ - if(!this.closing){ - move = this.bclient.rpoplpush(this.toKey(src), this.toKey(dst)); - }else{ - move = Promise.reject(); - } - } else if (this.closing || this.paused) { - move = Promise.resolve(); - } else if (this.getJobMoveCount()) { - move = this.whenCurrentMoveFinished().then(function() { - return _this.moveJob.apply(_this, args); - }); - }else{ - move = this.bclient.brpoplpush( - this.toKey(src), - this.toKey(dst), - Math.floor(this.LOCK_RENEW_TIME / 1000)); + + if(this.closing){ + return Promise.resolve(); } - return move.then(function(jobId){ - // - // Unfortunatelly this cannot be performed atomically, which will lead to a - // slight hazard for priority queues (will only affect its order). - // - if(jobId){ - return _this.client.zrem(_this.toKey('priority'), jobId).then(function(){ - return jobId; - }); - } - }, function(err){ - if(!_this.closing){ - throw err; + // + // Listen for new jobs, during moveToActive or after. + // + var newJobs = new Promise(function(resolve){ + resolve = resolve.bind(resolve, void 0) + _this.once('added', resolve); + _this.once('resumed', resolve); + _this.once('wait-finished', resolve); + }); + + return scripts.moveToActive(this).then(function(jobData){ + if(jobData){ + return Job.fromData(_this, jobData); + }else{ + return newJobs; } }); }; @@ -977,7 +922,7 @@ Queue.prototype.toKey = function(queueType){ return [this.keyPrefix, this.name, queueType].join(':'); }; -/*@function startCleaner +/*@function clean * * Cleans jobs from a queue. Similar to remove but keeps jobs within a certian * grace period. @@ -1024,26 +969,11 @@ Queue.prototype.clean = function (grace, type, limit) { Queue.prototype.whenCurrentJobsFinished = function(){ var _this = this; - return new Promise(function(resolve) { - var resolver; - var count = _this.processing + _this.retrieving; - - if(count === 0){ + _this.emit('wait-finished'); + return new Promise(function(resolve){ + _this.processing.finally(function(){ resolve(); - }else{ - resolver = _.after(count, function(){ - _this.removeListener('stalled', resolver); - _this.removeListener('completed', resolver); - _this.removeListener('failed', resolver); - _this.removeListener('no-job-retrieved', resolver); - resolve(); - }); - - _this.on('stalled', resolver); - _this.on('completed', resolver); - _this.on('failed', resolver); - _this.on('no-job-retrieved', resolver); - } + }); }); }; diff --git a/lib/scripts.js b/lib/scripts.js index 00bb08c3a..70962440d 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -23,6 +23,16 @@ function execScript(client, hash, lua, numberOfKeys){ return client[hash](args); } +function runScript(client, hash, lua, keys, args){ + if(!client[hash]){ + debuglog(hash, lua, keys, args); + client.defineCommand(hash, { numberOfKeys: keys.length, lua: lua }); + } + + var params = keys.concat(args); + return client[hash](params); +} + function isCommandDefined(client, hash){ return !!client[hash]; } @@ -62,7 +72,7 @@ var scripts = { var queue = job.queue; var lua = loadScriptIfNeeded(client, 'addJob'); - var keys = _.map(['wait', 'paused', 'meta-paused', 'jobs', 'id', 'delayed', 'priority'], function(name){ + var keys = _.map(['wait', 'paused', 'meta-paused', 'added', 'id', 'delayed', 'priority'], function(name){ return toKey(name); }); @@ -90,6 +100,37 @@ var scripts = { return execScript.apply(scripts, args); }, + pause: function(queue, pause) { + var lua = loadScriptIfNeeded(client, 'pause'); + + var src, dst; + if(pause){ + src = 'wait'; + dst = 'paused'; + }else{ + src = 'paused'; + dst = 'wait'; + } + + var keys = _.map([src, dst, 'meta-paused', 'paused'], function(name){ + return queue.toKey(name); + }); + + var args = [ + queue.client, + 'addJob', + lua, + keys.length, + keys[0], + keys[1], + keys[2], + keys[3], + pause ? 'paused' : 'resumed' + ] + + return execScript.apply(scripts, args); + }, + // TODO: DEPRECATE. move: function(job, src, target){ // TODO: Depending on the source we should use LREM or ZREM. @@ -97,7 +138,7 @@ var scripts = { var keys = _.map([ src, target, - job.jobId + job.id ], function(name){ return job.queue.toKey(name); } @@ -128,7 +169,7 @@ var scripts = { keys[0], keys[1], keys[2], - job.jobId, + job.id, job.returnvalue ? JSON.stringify(job.returnvalue) : '', parseInt(job.timestamp) ]; @@ -148,12 +189,12 @@ var scripts = { switch (result){ case -1: if(src){ - throw new Error('Missing Job ' + job.jobId + ' when trying to move from ' + src + ' to ' + target); + throw new Error('Missing Job ' + job.id + ' when trying to move from ' + src + ' to ' + target); } else { - throw new Error('Missing Job ' + job.jobId + ' when trying to remove it from ' + src); + throw new Error('Missing Job ' + job.id + ' when trying to remove it from ' + src); } case -2: - throw new Error('Cannot get lock for job ' + job.jobId + ' when trying to move from ' + src); + throw new Error('Cannot get lock for job ' + job.id + ' when trying to move from ' + src); default: return job.releaseLock(); } @@ -163,14 +204,45 @@ var scripts = { return scripts.move(job, 'active', removeOnComplete ? void 0 : 'completed'); }, - moveToFinished: function(job, val, propVal, shouldRemove, target, shouldLock){ + moveToActive: function(queue){ + var lua = loadScriptIfNeeded(queue.client, 'moveToActive'); + + var keys = _.map([ + 'wait', + 'active', + 'priority'], function(name){ + return queue.toKey(name); + } + ); + + var args = [ + queue.client, + 'moveToActive', + lua, + keys.length, + keys[0], + keys[1], + keys[2], + queue.toKey(''), + queue.token, + queue.LOCK_DURATION + ]; + + return execScript.apply(scripts, args).then(function(jobData){ + if(jobData && jobData.length){ + return array2hash(jobData); + } + }); + }, + + moveToFinished: function(job, val, propVal, shouldRemove, target, ignoreLock){ var queue = job.queue; var lua = loadScriptIfNeeded(queue.client, 'moveToFinished'); var keys = _.map([ 'active', target, - job.jobId], function(name){ + job.id], function(name){ return queue.toKey(name); } ); @@ -183,13 +255,16 @@ var scripts = { keys[0], keys[1], keys[2], - job.jobId, + job.id, Date.now(), propVal, val, + ignoreLock ? "0" : job.queue.token, shouldRemove ? "1" : "0" ]; + return execScript.apply(scripts, args); + /* if(shouldLock){ return wrapMove(job, function(){ return execScript.apply(scripts, args); @@ -197,14 +272,15 @@ var scripts = { }else{ return execScript.apply(scripts, args); } + */ }, - moveToCompleted: function(job, returnvalue, removeOnComplete){ - return scripts.moveToFinished(job, returnvalue, 'returnvalue', removeOnComplete, 'completed', true); + moveToCompleted: function(job, returnvalue, removeOnComplete, ignoreLock){ + return scripts.moveToFinished(job, returnvalue, 'returnvalue', removeOnComplete, 'completed', ignoreLock); }, - moveToFailed: function(job, failedReason, removeOnFailed){ - return scripts.moveToFinished(job, failedReason, 'failedReason', removeOnFailed, 'failed'); + moveToFailed: function(job, failedReason, removeOnFailed, ignoreLock){ + return scripts.moveToFinished(job, failedReason, 'failedReason', removeOnFailed, 'failed', ignoreLock); }, moveToSet: function(queue, set, jobId, context){ @@ -255,7 +331,6 @@ var scripts = { }); }, remove: function(queue, jobId){ - var lua = loadScriptIfNeeded(queue.client, 'removeJob'); var keys = _.map([ @@ -282,7 +357,55 @@ var scripts = { keys[4], keys[5], keys[6], - jobId + jobId, + queue.token + ]; + + return execScript.apply(scripts, args); + }, + + extendLock: function(queue, jobId){ + var lua = loadScriptIfNeeded(queue.client, 'extendLock'); + + var args = [ + queue.client, + 'extendLock', + lua, + 1, + queue.toKey(jobId) + ':lock', + queue.token, + queue.LOCK_DURATION + ]; + + return execScript.apply(scripts, args); + }, + + releaseLock: function(queue, jobId){ + var lua = loadScriptIfNeeded(queue.client, 'releaseLock'); + + var args = [ + queue.client, + 'releaseLock', + lua, + 1, + queue.toKey(jobId) + ':lock', + queue.token + ]; + + return execScript.apply(scripts, args); + }, + + takeLock: function(queue, job){ + var lua = loadScriptIfNeeded(queue.client, 'takeLock'); + + var args = [ + queue.client, + 'takeLock', + lua, + 1, + job.lockKey(), + queue.token, + queue.LOCK_DURATION ]; return execScript.apply(scripts, args); @@ -297,6 +420,7 @@ var scripts = { * is already locked and just reset the lock expiration. * @param {Boolean=false} ensureActive Ensures that the job is in the 'active' state. */ + /* takeLock: function(queue, job, renew, ensureActive){ var lock = job.lock; if (renew && !lock) { @@ -322,8 +446,8 @@ var scripts = { success ].join('\n'); }, - extraLockKeys: [job.queue.toKey('active'), queue.toKey(job.jobId)], - extraLockArgs: [job.jobId] + extraLockKeys: [job.queue.toKey('active'), queue.toKey(job.id)], + extraLockArgs: [job.id] }; redlock = new Redlock(queue.clients, _.extend(opts, queue.redlock)); } else { @@ -340,7 +464,8 @@ var scripts = { } }); }, - + */ +/* releaseLock: function(job){ var lock = job.lock; if (!lock) { @@ -348,6 +473,7 @@ var scripts = { } return lock.unlock() }, + */ /** It checks if the job in the top of the delay set should be moved back to the top of the wait queue (so that it will be processed as soon as possible) @@ -359,7 +485,7 @@ var scripts = { 'delayed', 'active', 'wait', - 'jobs'], function(name){ + 'added'], function(name){ return queue.toKey(name); }); @@ -398,20 +524,11 @@ var scripts = { moveUnlockedJobsToWait: function(queue){ var lua = loadScriptIfNeeded(queue.client, 'moveUnlockedJobsToWait'); - var args = [ - queue.client, - 'moveUnlockedJobsToWait', - lua, - 3, - queue.toKey('active'), - queue.toKey('wait'), - queue.toKey('failed'), - queue.MAX_STALLED_JOB_COUNT, - queue.toKey(''), - Date.now() - ]; - - return execScript.apply(scripts, args); + var keys = _.map(['active', 'wait', 'failed', 'added'], function(key){ + return queue.toKey(key); + }); + var args = [queue.MAX_STALLED_JOB_COUNT, queue.toKey(''), Date.now()]; + return runScript(queue.client, 'moveUnlockedJobsToWait', lua, keys, args); }, cleanJobsInSet: function(queue, set, ts, limit) { @@ -484,6 +601,25 @@ var scripts = { return execScript.apply(scripts, args); }, + retryJob: function(job){ + var queue = job.queue; + var jobId = job.id; + + var lua = loadScriptIfNeeded(queue.client, 'retryJob'); + + var keys = _.map(['active', 'wait', jobId, 'added'], function(name){ + return queue.toKey(name); + }); + + var pushCmd = (job.opts.lifo ? 'R' : 'L') + 'PUSH'; + + return runScript(queue.client, 'retryJob', lua, keys, [pushCmd, jobId]).then(function(result){ + if(result === -1){ + throw new Error('Missing Job ' + jobId + ' during retry'); + } + }); + }, + /** * Attempts to reprocess a job * @@ -522,11 +658,11 @@ var scripts = { var queue = job.queue; var keys = [ - queue.toKey(job.jobId), - queue.toKey(job.jobId) + ':lock', + queue.toKey(job.id), + queue.toKey(job.id) + ':lock', queue.toKey(options.state), queue.toKey('wait'), - queue.toKey('jobs') + queue.toKey('added') ]; var args = [ @@ -539,7 +675,7 @@ var scripts = { keys[2], keys[3], keys[4], - job.jobId + job.id ]; return execScript.apply(scripts, args); @@ -601,14 +737,22 @@ function wrapMove(job, move){ switch (result){ case -1: if(src){ - throw new Error('Missing Job ' + job.jobId + ' when trying to move from ' + src + ' to ' + target); + throw new Error('Missing Job ' + job.id + ' when trying to move from ' + src + ' to ' + target); } else { - throw new Error('Missing Job ' + job.jobId + ' when trying to remove it from ' + src); + throw new Error('Missing Job ' + job.id + ' when trying to remove it from ' + src); } case -2: - throw new Error('Cannot get lock for job ' + job.jobId + ' when trying to move from ' + src); + throw new Error('Cannot get lock for job ' + job.id + ' when trying to move from ' + src); default: return job.releaseLock(); } }); } + +function array2hash(arr){ + var obj = {}; + for(var i=0; i < arr.length; i+=2){ + obj[arr[i]] = arr[i+1] + } + return obj; +} diff --git a/lib/scripts/addJob.lua b/lib/scripts/addJob.lua index b7bf91f94..013aae793 100644 --- a/lib/scripts/addJob.lua +++ b/lib/scripts/addJob.lua @@ -18,7 +18,7 @@ KEYS[1] 'wait', KEYS[2] 'paused' KEYS[3] 'meta-paused' - KEYS[4] 'jobs' + KEYS[4] 'added' KEYS[5] 'id' KEYS[6] 'delayed' KEYS[7] 'priority' @@ -51,7 +51,7 @@ if redis.call("EXISTS", jobIdKey) == 1 then end -- Store the job. -redis.call("HMSET", jobIdKey, "name", ARGV[3], "data", ARGV[4], "opts", ARGV[5], "timestamp", ARGV[6], "delay", ARGV[7]) +redis.call("HMSET", jobIdKey,"id", jobId, "name", ARGV[3], "data", ARGV[4], "opts", ARGV[5], "timestamp", ARGV[6], "delay", ARGV[7]) -- Check if job is delayed if(ARGV[8] ~= "0") then diff --git a/lib/scripts/extendLock.lua b/lib/scripts/extendLock.lua new file mode 100644 index 000000000..f7a7cc996 --- /dev/null +++ b/lib/scripts/extendLock.lua @@ -0,0 +1,19 @@ +--[[ + Extend lock + + Input: + KEYS[1] 'lock', + + ARGV[1] token + ARGV[2] lock duration in milliseconds + + Output: + "OK" if lock extented succesfully. +]] + +if redis.call("GET", KEYS[1]) == ARGV[1] then + if redis.call("SET", KEYS[1], ARGV[1], "PX", ARGV[2]) then + return 1 + end +end +return 0 diff --git a/lib/scripts/moveToActive.lua b/lib/scripts/moveToActive.lua new file mode 100644 index 000000000..8530d28ee --- /dev/null +++ b/lib/scripts/moveToActive.lua @@ -0,0 +1,42 @@ +--[[ + Move next job to be processed to active, lock it and fetch its data. The job + may be delayed, in that case we need to move it to the delayed set instead. + + This operation guarantees that the worker owns the job during the locks + expiration time. The worker is responsible of keeping the lock fresh + so that no other worker picks this job again. + + Note: This command only works in non-distributed redis deployments. + + Input: + KEYS[1] wait key + KEYS[2] active key + KEYS[3] priority key + + ARGV[1] key prefix + ARGV[2] lock token + ARGV[3] lock duration in milliseconds +]] + +local jobId = redis.call("LINDEX", KEYS[1], -1) + +if jobId then + local jobKey = ARGV[1] .. jobId + local lockKey = jobKey .. ':lock' + + -- get a the lock + redis.call("SET", lockKey, ARGV[2], "PX", ARGV[3]) + redis.call("LREM", KEYS[1], 1, jobId) -- remove from wait + redis.call("ZREM", KEYS[3], jobId) -- remove from priority + redis.call("LPUSH", KEYS[2], jobId) -- push in active + return redis.call("HGETALL", jobKey) -- get job data +end + +--[[ + Release lock: + if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("del", KEYS[1]) + else + return 0 + end +]] diff --git a/lib/scripts/moveToFinished.lua b/lib/scripts/moveToFinished.lua index 3f24e72ec..58b8b7bf9 100644 --- a/lib/scripts/moveToFinished.lua +++ b/lib/scripts/moveToFinished.lua @@ -1,6 +1,8 @@ --[[ Move job from active to a finished status (completed o failed) A job can only be moved to completed if it was active. + The job must be locked before it can be moved to a finished status, + and the lock must be released in this script. Input: KEYS[1] active key @@ -11,9 +13,10 @@ ARGV[2] timestamp ARGV[3] msg property ARGV[4] return value / failed reason - ARGV[5] shouldRemove - ARGV[6] event channel - ARGV[7] event data (? maybe just send jobid). + ARGV[5] token + ARGV[6] shouldRemove + ARGV[7] event channel + ARGV[8] event data (? maybe just send jobid). Output: 0 OK @@ -22,13 +25,25 @@ Events: 'completed' ]] -if redis.call("EXISTS", KEYS[3]) == 1 then -- // Make sure job exists + +if redis.call("EXISTS", KEYS[3]) == 1 then -- // Make sure job exists + if ARGV[5] ~= "0" then + local lockKey = KEYS[3] .. ':lock' + if redis.call("GET", lockKey) == ARGV[5] then + redis.call("DEL", lockKey) + else + return -1 + end + end + + -- Remove from active list redis.call("LREM", KEYS[1], -1, ARGV[1]) -- Remove job? - if ARGV[5] == "1" then + if ARGV[6] == "1" then redis.call("DEL", KEYS[3]) else + -- Add to complete/failed set redis.call("ZADD", KEYS[2], ARGV[2], ARGV[1]) redis.call("HSET", KEYS[3], ARGV[3], ARGV[4]) -- "returnvalue" / "failedReason" end diff --git a/lib/scripts/moveUnlockedJobsToWait.lua b/lib/scripts/moveUnlockedJobsToWait.lua index 535ca8ab3..199e27361 100644 --- a/lib/scripts/moveUnlockedJobsToWait.lua +++ b/lib/scripts/moveUnlockedJobsToWait.lua @@ -7,6 +7,7 @@ back to wait to be re-processed. To prevent jobs from cycling endlessly between active and wait, (e.g. if the job handler keeps crashing), we limit the number stalled job recoveries to MAX_STALLED_JOB_COUNT. + DEPRECATED CASE: Case B) The job was just moved to 'active' from 'wait' and the worker that moved it hasn't gotten a lock yet, or died immediately before getting the lock (note that due to Redis limitations, the worker can't move the job and get the lock atomically - https://github.com/OptimalBits/bull/issues/258). @@ -17,7 +18,8 @@ Input: KEYS[1] 'active', KEYS[2] 'wait', - KEYS[3] 'failed', + KEYS[3] 'failed' + KEYS[4] 'added', ARGV[1] Max stalled job count ARGV[2] queue.toKey('') @@ -33,23 +35,19 @@ for _, job in ipairs(activeJobs) do if(redis.call("EXISTS", jobKey .. ":lock") == 0) then -- Remove from the active queue. redis.call("LREM", KEYS[1], 1, job) - local lockAcquired = redis.call("HGET", jobKey, "lockAcquired") - if(lockAcquired) then - -- If it was previously locked then we consider it 'stalled' (Case A above). If this job - -- has been stalled too many times, such as if it crashes the worker, then fail it. - local stalledCount = redis.call("HINCRBY", jobKey, "stalledCounter", 1) - if(stalledCount > MAX_STALLED_JOB_COUNT) then - redis.call("ZADD", KEYS[3], ARGV[3], job) - redis.call("HSET", jobKey, "failedReason", "job stalled more than allowable limit") - table.insert(failed, job) - else - -- Move the job back to the wait queue, to immediately be picked up by a waiting worker. - redis.call("RPUSH", KEYS[2], job) - table.insert(stalled, job) - end + + -- If it was previously locked then we consider it 'stalled' (Case A above). If this job + -- has been stalled too many times, such as if it crashes the worker, then fail it. + local stalledCount = redis.call("HINCRBY", jobKey, "stalledCounter", 1) + if(stalledCount > MAX_STALLED_JOB_COUNT) then + redis.call("ZADD", KEYS[3], ARGV[3], job) + redis.call("HSET", jobKey, "failedReason", "job stalled more than allowable limit") + table.insert(failed, job) else - -- Move the job back to the wait queue, to immediately be picked up by a waiting worker. - redis.call("RPUSH", KEYS[2], job) + -- Move the job back to the wait queue, to immediately be picked up by a waiting worker. + redis.call("RPUSH", KEYS[2], job) + table.insert(stalled, job) + redis.call("PUBLISH", KEYS[4], job) end end end diff --git a/lib/scripts/pause.lua b/lib/scripts/pause.lua new file mode 100644 index 000000000..090c3fb6e --- /dev/null +++ b/lib/scripts/pause.lua @@ -0,0 +1,18 @@ +--[[ + Pauses or resumes a queue globably. + + Note: This code is unnecessary complex, since it was used when we used BRPOPLPUSH. Currently + only a meta-paused key is necessary. + + +]] +if redis.call("EXISTS", KEYS[1]) == 1 then + redis.call("RENAME", KEYS[1], KEYS[2]) +end + +if ARGV[1] == "paused" then + redis.call("SET", KEYS[3], 1) +else + redis.call("DEL", KEYS[3]) +end +redis.call("PUBLISH", KEYS[4], ARGV[1]) diff --git a/lib/scripts/releaseLock.lua b/lib/scripts/releaseLock.lua new file mode 100644 index 000000000..697630ef0 --- /dev/null +++ b/lib/scripts/releaseLock.lua @@ -0,0 +1,17 @@ +--[[ + Release lock + + Input: + KEYS[1] 'lock', + + ARGV[1] token + ARGV[2] lock duration in milliseconds + + Output: + "OK" if lock extented succesfully. +]] +if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) +else + return 0 +end diff --git a/lib/scripts/removeJob.lua b/lib/scripts/removeJob.lua index f4ecf9bbd..5e3d3631b 100644 --- a/lib/scripts/removeJob.lua +++ b/lib/scripts/removeJob.lua @@ -1,6 +1,6 @@ --[[ - Move job from active to a finished status (completed o failed) - A job can only be moved to completed if it was active. + Remove a job from all the queues it may be in as well as all its data. + In order to be able to remove a job, it must be unlocked. Input: KEYS[1] 'active', @@ -12,15 +12,25 @@ KEYS[7] jobId ARGV[1] jobId + ARGV[2] lock token Events: 'removed' ]] -redis.call("LREM", KEYS[1], 0, ARGV[1]) -redis.call("LREM", KEYS[2], 0, ARGV[1]) -redis.call("ZREM", KEYS[3], ARGV[1]) -redis.call("LREM", KEYS[4], 0, ARGV[1]) -redis.call("ZREM", KEYS[5], ARGV[1]) -redis.call("ZREM", KEYS[6], ARGV[1]) -redis.call("DEL", KEYS[7]) +-- TODO PUBLISH global event 'removed' + +local lockKey = KEYS[7] .. ':lock' +local lock = redis.call("GET", lockKey) +if not lock then -- or (lock == ARGV[2])) then + redis.call("LREM", KEYS[1], 0, ARGV[1]) + redis.call("LREM", KEYS[2], 0, ARGV[1]) + redis.call("ZREM", KEYS[3], ARGV[1]) + redis.call("LREM", KEYS[4], 0, ARGV[1]) + redis.call("ZREM", KEYS[5], ARGV[1]) + redis.call("ZREM", KEYS[6], ARGV[1]) + redis.call("DEL", KEYS[7]) + return 1 +else + return 0 +end diff --git a/lib/scripts/retryJob.lua b/lib/scripts/retryJob.lua new file mode 100644 index 000000000..ba91c7024 --- /dev/null +++ b/lib/scripts/retryJob.lua @@ -0,0 +1,26 @@ +--[[ + Retries a failed job by moving it back to the wait queue. + + Input: + KEYS[1] 'active', + KEYS[2] 'wait' + KEYS[3] jobId + KEYS[4] 'added' + + pushCmd, + jobId + ARGV[1] pushCmd + ARGV[2] jobId + + Events: + 'prefix:added' +]] + +if redis.call("EXISTS", KEYS[3]) == 1 then + redis.call("LREM", KEYS[1], 0, ARGV[2]) + redis.call(ARGV[1], KEYS[2], ARGV[2]) + redis.call("PUBLISH", KEYS[4], ARGV[2]) + return 0 +else + return -1 +end diff --git a/lib/scripts/takeLock.lua b/lib/scripts/takeLock.lua new file mode 100644 index 000000000..dca6f77a0 --- /dev/null +++ b/lib/scripts/takeLock.lua @@ -0,0 +1,17 @@ +--[[ + Takes a lock + + Input: + KEYS[1] 'lock', + + ARGV[1] token + ARGV[2] lock duration in milliseconds + + Output: + "OK" if lock extented succesfully. +]] +if redis.call("SET", KEYS[1], ARGV[1], "NX", "PX", ARGV[2]) then + return 1 +else + return 0 +end diff --git a/lib/scripts/updateDelaySet.lua b/lib/scripts/updateDelaySet.lua index a92c78af3..d1c2b337c 100644 --- a/lib/scripts/updateDelaySet.lua +++ b/lib/scripts/updateDelaySet.lua @@ -5,7 +5,7 @@ KEYS[1] 'delayed' KEYS[2] 'active' KEYS[3] 'wait' - KEYS[4] 'jobs' event channel + KEYS[4] 'added' event channel ARGV[1] queue.toKey('') ARGV[2] delayed timestamp @@ -21,7 +21,7 @@ if (score ~= nil) then if (score <= tonumber(ARGV[2])) then redis.call("ZREM", KEYS[1], jobId) redis.call("LREM", KEYS[2], 0, jobId) - redis.call("LPUSH", KEYS[3], jobId) + redis.call("LPUSH", KEYS[3], jobId) -- not sure if it is better to move the job at the begining of the queue with LPUSH redis.call("PUBLISH", KEYS[4], jobId) redis.call("HSET", ARGV[1] .. jobId, "delay", 0) local nextTimestamp = redis.call("ZRANGE", KEYS[1], 0, 0, "WITHSCORES")[2] diff --git a/test/test_connection.js b/test/test_connection.js index 3a194c1a4..02a4b6062 100644 --- a/test/test_connection.js +++ b/test/test_connection.js @@ -27,14 +27,12 @@ describe('connection', function () { queue.close(); }).then(function() { done(); - }).catch(function(err){ - console.log(err); - }); + }).catch(done); // Simulate disconnect queue.on('ready', function(){ - queue.bclient.stream.end(); - queue.bclient.emit('error', new Error('ECONNRESET')); + queue.client.stream.end(); + queue.client.emit('error', new Error('ECONNRESET')); // add something to the queue queue.add({ 'foo': 'bar' }); @@ -57,13 +55,13 @@ describe('connection', function () { expect(runSpy.callCount).to.be(1); queue.add({ 'foo': 'bar' }); - queue.bclient.emit('end'); + queue.client.emit('end'); }); it.skip('should not try to reconnect when the blocking client triggers an "end" event and no process have been called', function (done) { var runSpy = sandbox.spy(queue, 'run'); - queue.bclient.emit('end'); + queue.client.emit('end'); setTimeout(function () { expect(runSpy.callCount).to.be(0); @@ -82,14 +80,12 @@ describe('connection', function () { queue.close().then(done, done); } count ++; - }).catch(function(err){ - console.log(err); - }); + }).catch(done); queue.on('completed', function(){ if(count === 1){ - queue.bclient.stream.end(); - queue.bclient.emit('error', new Error('ECONNRESET')); + queue.client.stream.end(); + queue.client.emit('error', new Error('ECONNRESET')); } }); @@ -97,7 +93,7 @@ describe('connection', function () { queue.add({ 'foo': 'bar' }); }); - queue.on('error', function (err) { + queue.on('error', function (/*err*/) { if(count === 1) { queue.add({ 'foo': 'bar' }); } diff --git a/test/test_job.js b/test/test_job.js index d381040e4..46d2c446c 100644 --- a/test/test_job.js +++ b/test/test_job.js @@ -7,7 +7,6 @@ var expect = require('expect.js'); var redis = require('ioredis'); var Promise = require('bluebird'); var uuid = require('uuid'); -var Redlock = require('bull-redlock'); describe('Job', function(){ @@ -42,7 +41,7 @@ describe('Job', function(){ }); it('returns a promise for the job', function () { - expect(job).to.have.property('jobId'); + expect(job).to.have.property('id'); expect(job).to.have.property('data'); }); @@ -51,8 +50,8 @@ describe('Job', function(){ }); it('saves the job in redis', function () { - return Job.fromId(queue, job.jobId).then(function(storedJob){ - expect(storedJob).to.have.property('jobId'); + return Job.fromId(queue, job.id).then(function(storedJob){ + expect(storedJob).to.have.property('id'); expect(storedJob).to.have.property('data'); expect(storedJob.data.foo).to.be.equal('bar'); @@ -64,7 +63,7 @@ describe('Job', function(){ it('should use the custom jobId if one is provided', function() { var customJobId = 'customjob'; return Job.create(queue, data, { jobId: customJobId }).then(function(createdJob){ - expect(createdJob.jobId).to.be.equal(customJobId); + expect(createdJob.id).to.be.equal(customJobId); }); }); @@ -77,7 +76,7 @@ describe('Job', function(){ queue.add({ foo: 'bar' }, { jobId: customJobId }); queue.on('completed', function(job) { - if (job.jobId == customJobId) { + if (job.id == customJobId) { done(); } }); @@ -91,25 +90,25 @@ describe('Job', function(){ return job.remove(); }) .then(function(job){ - return Job.fromId(queue, job.jobId); + return Job.fromId(queue, job.id); }) .then(function(storedJob){ expect(storedJob).to.be(null); }); }); - it('fails to remove a locked job', function() { + it('fails to remove a locked job', function(done) { return Job.create(queue, 1, {foo: 'bar'}).then(function(job) { return job.takeLock().then(function(lock) { - expect(lock).to.be.a(Redlock.Lock); + expect(lock).to.be.truthy; }).then(function() { - return Job.fromId(queue, job.jobId).then(function(job){ + return Job.fromId(queue, job.id).then(function(job){ return job.remove(); }); }).then(function() { throw new Error('Should not be able to remove a locked job'); - }).catch(function(err) { - expect(err.message).to.equal('Exceeded 0 attempts to lock the resource "bull:'+queue.name+':1:lock".'); + }).catch(function(/*err*/) { + done(); }); }); }); @@ -117,13 +116,15 @@ describe('Job', function(){ it('removes any job from active set', function() { return queue.add({ foo: 'bar' }).then(function(job) { // Simulate a job in active state but not locked - return queue.moveJob('wait', 'active').then(function() { + return queue.getNextJob().then(function() { return job.isActive().then(function(isActive) { expect(isActive).to.be(true); + return job.releaseLock() + }).then(function(){ return job.remove(); }); }).then(function() { - return Job.fromId(queue, job.jobId); + return Job.fromId(queue, job.id); }).then(function(stored) { expect(stored).to.be(null); return job.getState(); @@ -199,7 +200,7 @@ describe('Job', function(){ it('can take a lock', function(){ return job.takeLock().then(function(lockTaken){ - expect(lockTaken).to.be.a(Redlock.Lock); + expect(lockTaken).to.be.truthy; }).then(function(){ return job.releaseLock().then(function(lockReleased){ expect(lockReleased).to.not.exist; @@ -208,30 +209,28 @@ describe('Job', function(){ }); it('take an already taken lock', function(){ - var lock; return job.takeLock().then(function(lockTaken){ - lock = lockTaken; - expect(lockTaken).to.be.a(Redlock.Lock); + expect(lockTaken).to.be.truthy; }).then(function(){ return job.takeLock().then(function(lockTaken){ - expect(lockTaken).to.be(lock); + expect(lockTaken).to.be.truthy; }); }); }); it('can renew a previously taken lock', function(){ return job.takeLock().then(function(lockTaken){ - expect(lockTaken).to.be.a(Redlock.Lock); + expect(lockTaken).to.be.truthy; }).then(function(){ return job.renewLock().then(function(lockRenewed){ - expect(lockRenewed).to.be.a(Redlock.Lock); + expect(lockRenewed).to.be.truthy;; }); }); }); it('can release a lock', function(){ return job.takeLock().then(function(lockTaken){ - expect(lockTaken).to.be.a(Redlock.Lock); + expect(lockTaken).to.be.truthy; }).then(function(){ return job.releaseLock().then(function(lockReleased){ expect(lockReleased).to.not.exist; @@ -244,7 +243,7 @@ describe('Job', function(){ it('can set and get progress', function () { return Job.create(queue, {foo: 'bar'}).then(function(job){ return job.progress(42).then(function(){ - return Job.fromId(queue, job.jobId).then(function(storedJob){ + return Job.fromId(queue, job.id).then(function(storedJob){ expect(storedJob.progress()).to.be(42); }); }); @@ -258,8 +257,8 @@ describe('Job', function(){ return job.isCompleted().then(function(isCompleted){ expect(isCompleted).to.be(false); }).then(function(){ - return job.moveToCompleted('succeeded'); - }).then(function(){ + return job.moveToCompleted('succeeded', true); + }).then(function(/*moved*/){ return job.isCompleted().then(function(isCompleted){ expect(isCompleted).to.be(true); expect(job.returnvalue).to.be('succeeded'); @@ -399,7 +398,7 @@ describe('Job', function(){ return job.getState(); }).then(function(state) { expect(state).to.be('completed'); - return client.zrem(queue.toKey('completed'), job.jobId); + return client.zrem(queue.toKey('completed'), job.id); }).then(function(){ return job.moveToDelayed(Date.now() + 10000); }).then(function (){ @@ -409,7 +408,7 @@ describe('Job', function(){ return job.getState(); }).then(function(state) { expect(state).to.be('delayed'); - return client.zrem(queue.toKey('delayed'), job.jobId); + return client.zrem(queue.toKey('delayed'), job.id); }).then(function() { return job.moveToFailed(new Error('test'), true); }).then(function (){ @@ -419,7 +418,7 @@ describe('Job', function(){ return job.getState(); }).then(function(state) { expect(state).to.be('failed'); - return client.zrem(queue.toKey('failed'), job.jobId); + return client.zrem(queue.toKey('failed'), job.id); }).then(function(res) { expect(res).to.be(1); return job.getState(); @@ -427,7 +426,7 @@ describe('Job', function(){ expect(state).to.be('stuck'); return client.rpop(queue.toKey('wait')); }).then(function(){ - return client.lpush(queue.toKey('paused'), job.jobId); + return client.lpush(queue.toKey('paused'), job.id); }).then(function() { return job.isPaused(); }).then(function (isPaused) { @@ -437,7 +436,7 @@ describe('Job', function(){ expect(state).to.be('paused'); return client.rpop(queue.toKey('paused')); }).then(function() { - return client.lpush(queue.toKey('wait'), job.jobId); + return client.lpush(queue.toKey('wait'), job.id); }).then(function() { return job.isWaiting(); }).then(function (isWaiting) { @@ -480,7 +479,7 @@ describe('Job', function(){ return Promise.resolve(); }); queue.add({ foo: 'bar' }).then(function(job){ - return Promise.delay(1500).then(function(){ + return Promise.delay(500).then(function(){ return job.finished(); }) }).then(function(){ @@ -493,7 +492,7 @@ describe('Job', function(){ return Promise.reject(Error('test error')); }); queue.add({ foo: 'bar' }).then(function(job){ - return Promise.delay(1500).then(function(){ + return Promise.delay(500).then(function(){ return job.finished(); }); }).then(function(){ diff --git a/test/test_queue.js b/test/test_queue.js index 2db6b4f84..1bf819bab 100644 --- a/test/test_queue.js +++ b/test/test_queue.js @@ -46,13 +46,6 @@ describe('Queue', function () { testQueue.close(); }); - it('should call disconnect on the blocking client', function () { - var endSpy = sandbox.spy(testQueue.bclient, 'disconnect'); - return testQueue.close().then(function () { - expect(endSpy.calledOnce).to.be(true); - }); - }); - it('should call end on the event subscriber client', function (done) { testQueue.eclient.once('end', function () { done(); @@ -62,12 +55,10 @@ describe('Queue', function () { it('should resolve the promise when each client has disconnected', function () { expect(testQueue.client.status).to.be('ready'); - expect(testQueue.bclient.status).to.be('ready'); expect(testQueue.eclient.status).to.be('ready'); return testQueue.close().then(function () { expect(testQueue.client.status).to.be('end'); - expect(testQueue.bclient.status).to.be('end'); expect(testQueue.eclient.status).to.be('end'); }); }); @@ -104,7 +95,7 @@ describe('Queue', function () { }); testQueue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }); }); @@ -120,7 +111,7 @@ describe('Queue', function () { }); testQueue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }); }); @@ -132,13 +123,13 @@ describe('Queue', function () { var queue = new Queue('standard'); expect(queue.client.options.host).to.be('127.0.0.1'); - expect(queue.bclient.options.host).to.be('127.0.0.1'); + expect(queue.eclient.options.host).to.be('127.0.0.1'); expect(queue.client.options.port).to.be(6379); - expect(queue.bclient.options.port).to.be(6379); + expect(queue.eclient.options.port).to.be(6379); expect(queue.client.options.db).to.be(0); - expect(queue.bclient.options.db).to.be(0); + expect(queue.eclient.options.db).to.be(0); queue.close().then(done); }); @@ -147,13 +138,13 @@ describe('Queue', function () { var queue = new Queue('connstring', 'redis://127.0.0.1:6379'); expect(queue.client.options.host).to.be('127.0.0.1'); - expect(queue.bclient.options.host).to.be('127.0.0.1'); + expect(queue.eclient.options.host).to.be('127.0.0.1'); expect(queue.client.options.port).to.be(6379); - expect(queue.bclient.options.port).to.be(6379); + expect(queue.eclient.options.port).to.be(6379); expect(queue.client.options.db).to.be(0); - expect(queue.bclient.options.db).to.be(0); + expect(queue.eclient.options.db).to.be(0); queue.close().then(done); }); @@ -162,13 +153,13 @@ describe('Queue', function () { var queue = new Queue('connstring', '6379', '127.0.0.1'); expect(queue.client.options.host).to.be('127.0.0.1'); - expect(queue.bclient.options.host).to.be('127.0.0.1'); + expect(queue.eclient.options.host).to.be('127.0.0.1'); expect(queue.client.options.port).to.be(6379); - expect(queue.bclient.options.port).to.be(6379); + expect(queue.eclient.options.port).to.be(6379); expect(queue.client.condition.select).to.be(0); - expect(queue.bclient.condition.select).to.be(0); + expect(queue.eclient.condition.select).to.be(0); queue.close().then(done); }); @@ -177,13 +168,13 @@ describe('Queue', function () { var queue = new Queue('custom', { redis: { DB: 1 } }); expect(queue.client.options.host).to.be('127.0.0.1'); - expect(queue.bclient.options.host).to.be('127.0.0.1'); + expect(queue.eclient.options.host).to.be('127.0.0.1'); expect(queue.client.options.port).to.be(6379); - expect(queue.bclient.options.port).to.be(6379); + expect(queue.eclient.options.port).to.be(6379); expect(queue.client.options.db).to.be(1); - expect(queue.bclient.options.db).to.be(1); + expect(queue.eclient.options.db).to.be(1); queue.close().then(done); }); @@ -192,10 +183,10 @@ describe('Queue', function () { var queue = new Queue('custom', { redis: { host: 'localhost' } }); expect(queue.client.options.host).to.be('localhost'); - expect(queue.bclient.options.host).to.be('localhost'); + expect(queue.eclient.options.host).to.be('localhost'); expect(queue.client.options.db).to.be(0); - expect(queue.bclient.options.db).to.be(0); + expect(queue.eclient.options.db).to.be(0); queue.close().then(done); }); @@ -204,7 +195,7 @@ describe('Queue', function () { var queue = new Queue('using. dots. in.name.'); return queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).then(function () { queue.process(function (job, jobDone) { @@ -220,7 +211,7 @@ describe('Queue', function () { var queue = new Queue('foobar', '6379', 'localhost'); return queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).then(function () { queue.process(function (job, jobDone) { @@ -236,10 +227,10 @@ describe('Queue', function () { var queue = new Queue('q', 'redis://127.0.0.1', { keyPrefix: 'myQ' }); return queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); var client = new redis(); - return client.hgetall('myQ:q:' + job.jobId).then(function(result){ + return client.hgetall('myQ:q:' + job.id).then(function(result){ expect(result).to.not.be.null; }); }).then(function () { @@ -278,11 +269,11 @@ describe('Queue', function () { expect(queueQux.eclient).to.be.equal(subscriber); queueFoo.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).then(function(){ return queueQux.add({ qux: 'baz' }).then(function(job){ - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.qux).to.be('baz'); var completed = 0; @@ -337,7 +328,7 @@ describe('Queue', function () { }).catch(done); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }, done); }); @@ -349,12 +340,12 @@ describe('Queue', function () { }).catch(done); queue.add({ foo: 'bar' }, {removeOnComplete: true}).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }, done); queue.on('completed', function(job){ - queue.getJob(job.jobId).then(function(job){ + queue.getJob(job.id).then(function(job){ expect(job).to.be.equal(null); }).then(function(){ queue.getJobCounts().then(function(counts){ @@ -372,12 +363,12 @@ describe('Queue', function () { }).catch(done); queue.add({ foo: 'bar' }, {removeOnFail: true}).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }, done); queue.on('failed', function(job){ - queue.getJob(job.jobId).then(function(job){ + queue.getJob(job.id).then(function(job){ expect(job).to.be.equal(null); }).then(function(){ queue.getJobCounts().then(function(counts){ @@ -439,7 +430,7 @@ describe('Queue', function () { var total = 0; queue.process(function(job, jobDone){ - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.p).to.be(currentPriority); jobDone(); @@ -486,7 +477,7 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); @@ -504,7 +495,7 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); @@ -523,7 +514,7 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); @@ -531,7 +522,7 @@ describe('Queue', function () { expect(job).to.be.ok(); expect(data).to.be.eql(37); expect(job.returnvalue).to.be.eql(37); - queue.client.hget(queue.toKey(job.jobId), 'returnvalue').then(function (retval) { + queue.client.hget(queue.toKey(job.id), 'returnvalue').then(function (retval) { expect(JSON.parse(retval)).to.be.eql(37); done(); }); @@ -547,7 +538,7 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); @@ -565,7 +556,7 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); @@ -582,7 +573,7 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); @@ -593,10 +584,8 @@ describe('Queue', function () { }); it('process stalled jobs when starting a queue', function (done) { - // - // TODO: Investigate why this test is so slow. - // - this.timeout(12000); + + this.timeout(2000); utils.newQueue('test queue stalled').then(function (queueStalled) { queueStalled.LOCK_DURATION = 15; queueStalled.LOCK_RENEW_TIME = 5 @@ -609,7 +598,6 @@ describe('Queue', function () { Promise.all(jobs).then(function () { var afterJobsRunning = function () { var stalledCallback = sandbox.spy(); - return queueStalled.close(true).then(function () { return new Promise(function (resolve, reject) { utils.newQueue('test queue stalled').then(function (queue2) { @@ -635,7 +623,6 @@ describe('Queue', function () { }; var onceRunning = _.once(afterJobsRunning); - queueStalled.process(function () { onceRunning(); return Promise.delay(150); @@ -680,7 +667,7 @@ describe('Queue', function () { }); queue.add('myname', { foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); @@ -707,7 +694,7 @@ describe('Queue', function () { }); queue.add('myname', { foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).then(function(){ return queue.add('myname2', { baz: 'qux' }); @@ -731,16 +718,16 @@ describe('Queue', function () { }); it('fails job if missing named process', function (done) { - queue.process(function (job) { + queue.process(function (/*job*/) { done(Error('should not process this job')) }); - queue.once('failed', function(err){ + queue.once('failed', function(/*err*/){ done(); }); queue.add('myname', { foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }); }); @@ -801,7 +788,7 @@ describe('Queue', function () { var processes = []; stalledQueues.forEach(function(queue){ - queue.on('error', function(err){ + queue.on('error', function(/*err*/){ // // Swallow errors produced by the disconnect // @@ -825,7 +812,7 @@ describe('Queue', function () { queue.process(function (job, jobDone) { expect(job.data.foo).to.be.equal('bar'); - if (addedJob.jobId !== job.jobId) { + if (addedJob.id !== job.id) { err = new Error('Processed job id does not match that of added job'); } setTimeout(jobDone, 500); @@ -852,22 +839,27 @@ describe('Queue', function () { queue2.close().then(done); }); - queue2.LOCK_RENEW_TIME = 500; + queue2.LOCK_RENEW_TIME = 5000; + queue2.LOCK_DURATION = 500; + queue2.STALLED_JOB_CHECK_INTERVAL = 1000; queue2.on('completed', function () { + var client = new redis(); + client.multi() + .zrem(queue2.toKey('completed'), 1) + .lpush(queue2.toKey('active'), 1) + .exec(); + client.quit(); collect(); }); queue2.process(function (job, jobDone) { expect(job.data.foo).to.be.equal('bar'); jobDone(); - var client = new redis(); - client.zrem(queue2.toKey('completed'), 1); - client.lpush(queue2.toKey('active'), 1); }); queue2.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }).catch(done); }); @@ -881,14 +873,14 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }, function (err) { done(err); }); queue.once('failed', function (job, err) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); expect(err).to.be.eql(jobError); done(); @@ -904,14 +896,14 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }, function (err) { done(err); }); queue.once('failed', function (job, err) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); expect(err).to.be.eql(jobError); done(); @@ -946,14 +938,14 @@ describe('Queue', function () { }); queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }, function (err) { done(err); }); queue.once('failed', function (job, err) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); expect(err).to.be.eql(jobError); done(); @@ -977,7 +969,7 @@ describe('Queue', function () { }; queue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }, function (err) { done(err); @@ -998,14 +990,14 @@ describe('Queue', function () { client.on('ready', function () { client.on('message', function (channel, message) { - expect(channel).to.be.equal(retryQueue.toKey('jobs')); + expect(channel).to.be.equal(retryQueue.toKey('added')); expect(parseInt(message, 10)).to.be.a('number'); messages++; }); - client.subscribe(retryQueue.toKey('jobs')); + client.subscribe(retryQueue.toKey('added')); retryQueue.add({ foo: 'bar' }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); }); }); @@ -1019,7 +1011,7 @@ describe('Queue', function () { }); retryQueue.once('failed', function (job, err) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.foo).to.be('bar'); expect(err.message).to.be.eql('Not even!'); failedOnce = true; @@ -1048,7 +1040,7 @@ describe('Queue', function () { return Promise.all(added) .then(queue.count.bind(queue)) .then(function (count) { - expect(count).to.be(100); + expect(count).to.be(maxJobs); }) .then(queue.empty.bind(queue)) .then(queue.count.bind(queue)) @@ -1164,15 +1156,22 @@ describe('Queue', function () { it('should wait until active jobs are finished before resolving pause', function (done) { var queue = utils.buildQueue(); - queue.process(function (job, completed) { - setTimeout(completed, 200); - }); + var startProcessing = new Promise(function(resolve){ + queue.process(function (/*job*/) { + resolve(); + return Promise.delay(200); + }); + }) queue.on('ready', function () { var jobs = []; for (var i = 0; i < 10; i++) { jobs.push(queue.add(i)); } + // + // Add start processing so that we can test that pause waits for this job to be completed. + // + jobs.push(startProcessing); Promise.all(jobs).then(function () { return queue.pause(true).then(function () { var active = queue.getJobCountByTypes(['active']).then(function (count) { @@ -1186,9 +1185,7 @@ describe('Queue', function () { expect(count).to.be(9); return null; }); - return Promise.all([active, paused]); - }).then(function () { return queue.add({}); }).then(function () { @@ -1254,18 +1251,20 @@ describe('Queue', function () { it('should wait for blocking job retrieval to complete before pausing locally', function() { var queue = utils.buildQueue(); - queue.process(function(job, jobDone) { - setTimeout(jobDone, 200); - }); - return new Promise(function(resolve) { - queue.on('ready', resolve); - }).then(function() { - return queue.add(1).then(function(){ - return queue.pause(true); - }).then(function(){ - queue.add(2); + var startsProcessing = new Promise(function(resolve){ + queue.process(function(/*job*/) { + resolve(); + return Promise.delay(200) }); + }); + + return queue.add(1).then(function(){ + return startsProcessing; + }).then(function(){ + return queue.pause(true); + }).then(function(){ + return queue.add(2); }).then(function() { var active = queue.getJobCountByTypes(['active']).then(function(count) { expect(count).to.be(0); @@ -1290,11 +1289,11 @@ describe('Queue', function () { var queue = new Queue('test pub sub'); client.on('ready', function () { client.on('message', function (channel, message) { - expect(channel).to.be.equal(queue.toKey('jobs')); + expect(channel).to.be.equal(queue.toKey('added')); expect(parseInt(message, 10)).to.be.a('number'); queue.close().then(done, done); }); - client.subscribe(queue.toKey('jobs')); + client.subscribe(queue.toKey('added')); queue.add({ test: 'stuff' }); }); }); @@ -1349,7 +1348,7 @@ describe('Queue', function () { expect(parseInt(message, 10)).to.be.a('number'); publishHappened = true; }); - client.subscribe(queue.toKey('jobs')); + client.subscribe(queue.toKey('added')); }); queue.process(function (job, jobDone) { @@ -1372,7 +1371,7 @@ describe('Queue', function () { queue.on('ready', function () { queue.add({ delayed: 'foobar' }, { delay: delay }).then(function (job) { - expect(job.jobId).to.be.ok(); + expect(job.id).to.be.ok(); expect(job.data.delayed).to.be('foobar'); expect(job.delay).to.be(delay); }); @@ -1388,29 +1387,25 @@ describe('Queue', function () { done(err); }); - queue.on('ready', function () { - - queue.process(function (job, jobDone) { - order++; - expect(order).to.be.equal(job.data.order); - jobDone(); - if (order === 10) { - queue.close().then(done, done); - } - }); - - queue.add({ order: 1 }, { delay: 100 }); - queue.add({ order: 6 }, { delay: 600 }); - queue.add({ order: 10 }, { delay: 1000 }); - queue.add({ order: 2 }, { delay: 200 }); - queue.add({ order: 9 }, { delay: 900 }); - queue.add({ order: 5 }, { delay: 500 }); - queue.add({ order: 3 }, { delay: 300 }); - queue.add({ order: 7 }, { delay: 700 }); - queue.add({ order: 4 }, { delay: 400 }); - queue.add({ order: 8 }, { delay: 800 }); - + queue.process(function (job, jobDone) { + order++; + expect(order).to.be.equal(job.data.order); + jobDone(); + if (order === 10) { + queue.close().then(done, done); + } }); + + queue.add({ order: 1 }, { delay: 100 }); + queue.add({ order: 6 }, { delay: 600 }); + queue.add({ order: 10 }, { delay: 1000 }); + queue.add({ order: 2 }, { delay: 200 }); + queue.add({ order: 9 }, { delay: 900 }); + queue.add({ order: 5 }, { delay: 500 }); + queue.add({ order: 3 }, { delay: 300 }); + queue.add({ order: 7 }, { delay: 700 }); + queue.add({ order: 4 }, { delay: 400 }); + queue.add({ order: 8 }, { delay: 800 }); }); it('should process delayed jobs in correct order even in case of restart', function (done) { @@ -1765,7 +1760,7 @@ describe('Queue', function () { .then(function () { return queue.clean(0).then(function () { return job.retry().catch(function (err) { - expect(err.message).to.equal('Couldn\'t retry job: The job doesn\'t exist'); + expect(err.message).to.equal(Queue.ErrorMessages.RETRY_JOB_NOT_EXIST); }); }); }) @@ -1815,7 +1810,7 @@ describe('Queue', function () { }) .then(function () { return job.retry().catch(function (err) { - expect(err.message).to.equal('Couldn\'t retry job: The job has been already retried or has not failed'); + expect(err.message).to.equal(Queue.ErrorMessages.RETRY_JOB_NOT_FAILED); }); }) .then(function () { @@ -1841,14 +1836,16 @@ describe('Queue', function () { var addedHandler = _.once(function (job) { expect(job.data.foo).to.equal('bar'); - job.retry().catch(function (err) { - expect(err.message).to.equal('Couldn\'t retry job: The job has been already retried or has not failed'); - return null; - }).then(done, done); + Promise.delay(100).then(function(){ + job.retry().catch(function (err) { + expect(err.message).to.equal(Queue.ErrorMessages.RETRY_JOB_IS_LOCKED); + return null; + }).then(done, done); + }); }); queue.process(function (job, jobDone) { - return Promise.delay(200).then(jobDone); + return Promise.delay(300).then(jobDone); }); queue.add({ foo: 'bar' }).then(addedHandler); }); @@ -1908,9 +1905,9 @@ describe('Queue', function () { it('should get a specific job', function (done) { var data = { foo: 'sup!' }; queue.add(data).then(function (job) { - queue.getJob(job.jobId).then(function (returnedJob) { + queue.getJob(job.id).then(function (returnedJob) { expect(returnedJob.data).to.eql(data); - expect(returnedJob.jobId).to.be(job.jobId); + expect(returnedJob.id).to.be(job.id); done(); }); }); @@ -2049,9 +2046,11 @@ describe('Queue', function () { }).catch(done); })); - queue.add({ foo: 1 }); - queue.add({ foo: 2 }); - queue.add({ foo: 3 }); + queue.add({ foo: 1 }).then(function(){ + return queue.add({ foo: 2 }); + }).then(function(){ + return queue.add({ foo: 3 }); + }); }); it('should return subset of jobs when setting a negative range', function (done) { diff --git a/test/utils.js b/test/utils.js index 2cdd0bb70..f4bfeb809 100644 --- a/test/utils.js +++ b/test/utils.js @@ -9,7 +9,6 @@ var queues = []; function simulateDisconnect(queue){ queue.client.disconnect(); - queue.bclient.disconnect(); queue.eclient.disconnect(); } From dc5248d8520ccccd88371b04a94913a768dc8086 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Thu, 13 Apr 2017 11:22:12 +0200 Subject: [PATCH 04/17] store one processing promise per concurrency level --- lib/queue.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/queue.js b/lib/queue.js index 02dd6e309..fbb5d1bb3 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -157,7 +157,7 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ this.handlers = {}; this.delayTimer = null; - this.processing = Promise.resolve(); + this.processing = []; this.retrieving = 0; this.LOCK_DURATION = LOCK_DURATION; @@ -277,7 +277,6 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ // Bind these methods to avoid constant rebinding and/or creating closures // in processJobs etc. this.moveUnlockedJobsToWait = this.moveUnlockedJobsToWait.bind(this); - this.processJobs = this.processJobs.bind(this); this.processJob = this.processJob.bind(this); this.getJobFromId = Job.fromId.bind(null, this); }; @@ -563,7 +562,9 @@ Queue.prototype.run = function(concurrency){ return this.moveUnlockedJobsToWait().then(function(){ while(concurrency--){ - promises.push(new Promise(_this.processJobs)); + promises.push(new Promise(function(resolve, reject){ + _this.processJobs(concurrency, resolve, reject); + })); } _this.startMoveUnlockedJobsToWait(); @@ -649,13 +650,13 @@ Queue.prototype.startMoveUnlockedJobsToWait = function() { // Idea to be able to know when a processing promise is finished, just store the promise -Queue.prototype.processJobs = function(resolve, reject){ +Queue.prototype.processJobs = function(index, resolve, reject){ var _this = this; - var processJobs = this.processJobs.bind(this, resolve, reject); + var processJobs = this.processJobs.bind(this, index, resolve, reject); process.nextTick(function(){ if(!_this.closing){ (_this.paused || Promise.resolve()).then(function(){ - return _this.processing = _this.getNextJob() + return _this.processing[index] = _this.getNextJob() .then(_this.processJob) .then(processJobs, function(err){ _this.emit('error', err, 'Error processing job'); @@ -971,7 +972,7 @@ Queue.prototype.whenCurrentJobsFinished = function(){ _this.emit('wait-finished'); return new Promise(function(resolve){ - _this.processing.finally(function(){ + Promise.all(_this.processing).finally(function(){ resolve(); }); }); From 7fedea6f96a1f3908234a2ed4dbabeffdd68227a Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Thu, 13 Apr 2017 12:17:18 +0200 Subject: [PATCH 05/17] clean ups and refactoring in lua scripts --- lib/scripts.js | 338 +++-------------------------------- lib/scripts/addJob.lua | 2 +- lib/scripts/isJobInList.lua | 20 +++ lib/scripts/reprocessJob.lua | 43 +++++ 4 files changed, 89 insertions(+), 314 deletions(-) create mode 100644 lib/scripts/isJobInList.lua create mode 100644 lib/scripts/reprocessJob.lua diff --git a/lib/scripts.js b/lib/scripts.js index 70962440d..e9d39234a 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -46,25 +46,10 @@ function loadScriptIfNeeded(client, name){ } var scripts = { - _isJobInList: function(keyVar, argVar, operator) { - keyVar = keyVar || 'KEYS[1]'; - argVar = argVar || 'ARGV[1]'; - operator = operator || 'return'; - return [ - 'local function item_in_list (list, item)', - ' for _, v in pairs(list) do', - ' if v == item then', - ' return 1', - ' end', - ' end', - ' return nil', - 'end', - ['local items = redis.call("LRANGE",', keyVar, ' , 0, -1)'].join(''), - [operator, ' item_in_list(items, ', argVar, ')'].join('') - ].join('\n'); - }, isJobInList: function(client, listKey, jobId){ - return execScript(client, 'isJobInList', this._isJobInList(), 1, listKey, jobId).then(function(result){ + var lua = loadScriptIfNeeded(client, 'isJobInList'); + + return runScript(client, 'isJobInList', lua, [listKey], [jobId]).then(function(result){ return result === 1; }); }, @@ -77,14 +62,6 @@ var scripts = { }); var args = [ - client, - 'addJob', - lua, - keys.length - ] - - args.push.apply(args, keys); - args.push.apply(args, [ toKey(''), _.isUndefined(opts.customJobId) ? "" : opts.customJobId, job.name, @@ -95,9 +72,9 @@ var scripts = { job.delay ? job.timestamp + job.delay : "0", opts.priority || 0, opts.lifo ? "LIFO" : "FIFO" - ]); + ]; - return execScript.apply(scripts, args); + return runScript(client, 'addJob', lua, keys, args); }, pause: function(queue, pause) { @@ -116,22 +93,12 @@ var scripts = { return queue.toKey(name); }); - var args = [ - queue.client, - 'addJob', - lua, - keys.length, - keys[0], - keys[1], - keys[2], - keys[3], - pause ? 'paused' : 'resumed' - ] - - return execScript.apply(scripts, args); + return runScript(queue.client, 'pause', lua, keys, [pause ? 'paused' : 'resumed']) }, + // // TODO: DEPRECATE. + // move: function(job, src, target){ // TODO: Depending on the source we should use LREM or ZREM. // TODO: Depending on the target we should use LPUSH, SADD, etc. @@ -200,9 +167,6 @@ var scripts = { } }); }, - _moveToCompleted: function(job, removeOnComplete){ - return scripts.move(job, 'active', removeOnComplete ? void 0 : 'completed'); - }, moveToActive: function(queue){ var lua = loadScriptIfNeeded(queue.client, 'moveToActive'); @@ -216,19 +180,12 @@ var scripts = { ); var args = [ - queue.client, - 'moveToActive', - lua, - keys.length, - keys[0], - keys[1], - keys[2], queue.toKey(''), queue.token, queue.LOCK_DURATION ]; - return execScript.apply(scripts, args).then(function(jobData){ + return runScript(queue.client, 'moveToActive', lua, keys, args).then(function(jobData){ if(jobData && jobData.length){ return array2hash(jobData); } @@ -248,13 +205,6 @@ var scripts = { ); var args = [ - queue.client, - 'moveToFinished', - lua, - keys.length, - keys[0], - keys[1], - keys[2], job.id, Date.now(), propVal, @@ -263,16 +213,7 @@ var scripts = { shouldRemove ? "1" : "0" ]; - return execScript.apply(scripts, args); - /* - if(shouldLock){ - return wrapMove(job, function(){ - return execScript.apply(scripts, args); - }); - }else{ - return execScript.apply(scripts, args); - } - */ + return runScript(queue.client, 'moveToFinished', lua, keys, args); }, moveToCompleted: function(job, returnvalue, removeOnComplete, ignoreLock){ @@ -312,24 +253,13 @@ var scripts = { } ); - var args = [ - queue.client, - 'moveToSet', - lua, - keys.length, - keys[0], - keys[1], - keys[2], - JSON.stringify(context), - jobId - ]; - - return execScript.apply(scripts, args).then(function(result){ + return runScript(queue.client, 'moveToSet', lua, keys, [JSON.stringify(context), jobId]).then(function(result){ if(result === -1){ throw new Error('Missing Job ' + jobId + ' when trying to move from active to ' + set); } }); }, + remove: function(queue, jobId){ var lua = loadScriptIfNeeded(queue.client, 'removeJob'); @@ -345,135 +275,27 @@ var scripts = { } ); - var args = [ - queue.client, - 'removeJob', - lua, - keys.length, - keys[0], - keys[1], - keys[2], - keys[3], - keys[4], - keys[5], - keys[6], - jobId, - queue.token - ]; - - return execScript.apply(scripts, args); + return runScript(queue.client, 'removeJob', lua, keys, [jobId, queue.token]); }, extendLock: function(queue, jobId){ var lua = loadScriptIfNeeded(queue.client, 'extendLock'); - var args = [ - queue.client, - 'extendLock', - lua, - 1, - queue.toKey(jobId) + ':lock', - queue.token, - queue.LOCK_DURATION - ]; - - return execScript.apply(scripts, args); + return runScript(queue.client, 'extendLock', lua, [queue.toKey(jobId) + ':lock'], [queue.token, queue.LOCK_DURATION]); }, releaseLock: function(queue, jobId){ var lua = loadScriptIfNeeded(queue.client, 'releaseLock'); - var args = [ - queue.client, - 'releaseLock', - lua, - 1, - queue.toKey(jobId) + ':lock', - queue.token - ]; - - return execScript.apply(scripts, args); + return runScript(queue.client, 'releaseLock', lua, [queue.toKey(jobId) + ':lock'], [queue.token]); }, takeLock: function(queue, job){ var lua = loadScriptIfNeeded(queue.client, 'takeLock'); - var args = [ - queue.client, - 'takeLock', - lua, - 1, - job.lockKey(), - queue.token, - queue.LOCK_DURATION - ]; - - return execScript.apply(scripts, args); + return runScript(queue.client, 'takeLock', lua, [job.lockKey()], [queue.token, queue.LOCK_DURATION]); }, - /** - * Gets a lock for a job. - * - * @param {Queue} queue The queue for the job - * @param {Job} job The job - * @param {Boolean=false} renew Whether to renew to lock, meaning it will assume the job - * is already locked and just reset the lock expiration. - * @param {Boolean=false} ensureActive Ensures that the job is in the 'active' state. - */ - /* - takeLock: function(queue, job, renew, ensureActive){ - var lock = job.lock; - if (renew && !lock) { - throw new Error('Unable to renew nonexisting lock'); - } - if (renew) { - return lock.extend(queue.LOCK_DURATION); - } - if (lock) { - return Promise.resolve(lock); - } - var redlock; - if (ensureActive) { - var isJobInList = this._isJobInList('KEYS[2]', 'ARGV[3]', 'if'); - var lockAcquired = ['and redis.call("HSET", KEYS[3], "lockAcquired", "1")'].join(''); - var success = 'then return 1 else return 0 end'; - var opts = { - lockScript: function(lockScript) { - return [ - isJobInList, - lockScript.replace('return', 'and'), - lockAcquired, - success - ].join('\n'); - }, - extraLockKeys: [job.queue.toKey('active'), queue.toKey(job.id)], - extraLockArgs: [job.id] - }; - redlock = new Redlock(queue.clients, _.extend(opts, queue.redlock)); - } else { - redlock = new Redlock(queue.clients, queue.redlock); - } - return redlock.lock(job.lockKey(), queue.LOCK_DURATION).catch(function(err){ - // - // Failing to lock due to already locked is not an error. - // - if(err.name != 'LockError'){ - throw err; - }else{ - queue.emit('error', err, 'Could not get the lock'); - } - }); - }, - */ -/* - releaseLock: function(job){ - var lock = job.lock; - if (!lock) { - throw new Error('Unable to release nonexisting lock'); - } - return lock.unlock() - }, - */ /** It checks if the job in the top of the delay set should be moved back to the top of the wait queue (so that it will be processed as soon as possible) @@ -489,37 +311,18 @@ var scripts = { return queue.toKey(name); }); - var args = [ - queue.client, - 'updateDelaySet', - lua, - keys.length, - keys[0], - keys[1], - keys[2], - keys[3], - queue.toKey(''), - delayedTimestamp - ]; + var args = [queue.toKey(''), delayedTimestamp]; - return execScript.apply(scripts, args); + return runScript(queue.client, 'updateDelaySet', lua, keys, args); }, /** - * Looks for unlocked jobs in the active queue. There are two circumstances in which a job - * would be in 'active' but NOT have a job lock: + * Looks for unlocked jobs in the active queue. * - * Case A) The job was being worked on, but the worker process died and it failed to renew the lock. + * The job was being worked on, but the worker process died and it failed to renew the lock. * We call these jobs 'stalled'. This is the most common case. We resolve these by moving them * back to wait to be re-processed. To prevent jobs from cycling endlessly between active and wait, * (e.g. if the job handler keeps crashing), we limit the number stalled job recoveries to MAX_STALLED_JOB_COUNT. - - * Case B) The job was just moved to 'active' from 'wait' and the worker that moved it hasn't gotten - * a lock yet, or died immediately before getting the lock (note that due to Redis limitations, the - * worker can't move the job and get the lock atomically - https://github.com/OptimalBits/bull/issues/258). - * For this case we also move the job back to 'wait' for reprocessing, but don't consider it 'stalled' - * since the job had never been started. This case is much rarer than Case A due to the very small - * timing window in which it must occur. */ moveUnlockedJobsToWait: function(queue){ var lua = loadScriptIfNeeded(queue.client, 'moveUnlockedJobsToWait'); @@ -531,6 +334,7 @@ var scripts = { return runScript(queue.client, 'moveUnlockedJobsToWait', lua, keys, args); }, + // TODO: Refacto into lua script cleanJobsInSet: function(queue, set, ts, limit) { var command; var removeCommand; @@ -635,26 +439,7 @@ var scripts = { * -2 means the job was not found in the expected set */ reprocessJob: function(job, options) { - var push = (job.opts.lifo ? 'R' : 'L') + 'PUSH'; - - var script = [ - 'if (redis.call("EXISTS", KEYS[1]) == 1) then', - ' if (redis.call("EXISTS", KEYS[2]) == 0) then', - ' if (redis.call("ZREM", KEYS[3], ARGV[1]) == 1) then', - ' redis.call("' + push + '", KEYS[4], ARGV[1])', - ' redis.call("PUBLISH", KEYS[5], ARGV[1])', - ' return 1', - ' else', - ' return -2', - ' end', - ' else', - ' return -1', - ' end', - 'else', - ' return 0', - 'end' - ].join('\n'); - + var lua = loadScriptIfNeeded(job.queue.client, 'reprocessJob'); var queue = job.queue; var keys = [ @@ -666,89 +451,16 @@ var scripts = { ]; var args = [ - queue.client, - 'retryJob', - script, - 5, - keys[0], - keys[1], - keys[2], - keys[3], - keys[4], - job.id + job.id, + (job.opts.lifo ? 'R' : 'L') + 'PUSH' ]; - return execScript.apply(scripts, args); + return runScript(queue.client, 'reprocessJob', lua, keys, args); } }; -/* -Queue.prototype.empty = function(){ - var _this = this; - - // Get all jobids and empty all lists atomically. - var multi = this.multi(); - - multi.lrange(this.toKey('wait'), 0, -1); - multi.lrange(this.toKey('paused'), 0, -1); - multi.del(this.toKey('wait')); - multi.del(this.toKey('paused')); - multi.del(this.toKey('meta-paused')); - multi.del(this.toKey('delayed')); - - return multi.exec().spread(function(waiting, paused){ - var jobKeys = (paused.concat(waiting)).map(_this.toKey, _this); - - if(jobKeys.length){ - multi = _this.multi(); - - multi.del.apply(multi, jobKeys); - return multi.exec(); - } - }); -}; -*/ -/** - * KEYS: - * 0 - wait - * 1 - paused - * 2 - meta-paused - * 3 - delayed - */ -var emptyScript = [ - -] - module.exports = scripts; -function wrapMove(job, move){ - var returnLockOrErrorCode = function(lock) { - return lock ? move() : -2; - }; - var throwUnexpectedErrors = function(err) { - if (!(err instanceof Redlock.LockError)) { - throw err; - } - }; - - return job.takeLock(!!job.lock) - .then(returnLockOrErrorCode, throwUnexpectedErrors) - .then(function(result){ - switch (result){ - case -1: - if(src){ - throw new Error('Missing Job ' + job.id + ' when trying to move from ' + src + ' to ' + target); - } else { - throw new Error('Missing Job ' + job.id + ' when trying to remove it from ' + src); - } - case -2: - throw new Error('Cannot get lock for job ' + job.id + ' when trying to move from ' + src); - default: - return job.releaseLock(); - } - }); -} - function array2hash(arr){ var obj = {}; for(var i=0; i < arr.length; i+=2){ diff --git a/lib/scripts/addJob.lua b/lib/scripts/addJob.lua index 013aae793..b018ce84a 100644 --- a/lib/scripts/addJob.lua +++ b/lib/scripts/addJob.lua @@ -37,7 +37,7 @@ Events: 'waiting' ]] -local jobCounter = redis.call("INCR", KEYS[5]) -- Why doing this allways ? +local jobCounter = redis.call("INCR", KEYS[5]) local jobId if ARGV[2] == "" then jobId = jobCounter diff --git a/lib/scripts/isJobInList.lua b/lib/scripts/isJobInList.lua new file mode 100644 index 000000000..8b1f5f324 --- /dev/null +++ b/lib/scripts/isJobInList.lua @@ -0,0 +1,20 @@ +--[[ + Checks if job is in a given list. + + Input: + KEYS[1] + ARGV[1] + + Output: + 1 if element found in the list. +]] +local function item_in_list (list, item) + for _, v in pairs(list) do + if v == item then + return 1 + end + end + return nil +end +local items = redis.call("LRANGE", KEYS[1] , 0, -1) +return item_in_list(items, ARGV[1]) diff --git a/lib/scripts/reprocessJob.lua b/lib/scripts/reprocessJob.lua new file mode 100644 index 000000000..9fbf72f97 --- /dev/null +++ b/lib/scripts/reprocessJob.lua @@ -0,0 +1,43 @@ +--[[ + Attempts to reprocess a job + + Input: + KEYS[1] job key + KEYS[2] job lock key + KEYS[3] job state + KEYS[4] wait key + KEYS[5] added key + + var keys = [ + queue.toKey(job.id), + queue.toKey(job.id) + ':lock', + queue.toKey(options.state), + queue.toKey('wait'), + queue.toKey('added') + ]; + + Output: + 1 means the operation was a success + 0 means the job does not exist + -1 means the job is currently locked and can't be retried. + -2 means the job was not found in the expected set. + + Events: + emits 'added' if succesfully moved job to wait. +]] +if (redis.call("EXISTS", KEYS[1]) == 1) then + if (redis.call("EXISTS", KEYS[2]) == 0) then + if (redis.call("ZREM", KEYS[3], ARGV[1]) == 1) then + redis.call(ARGV[2], KEYS[4], ARGV[1]) + redis.call("PUBLISH", KEYS[5], ARGV[1]) + return 1 + else + return -2 + end + else + return -1 + end +else + return 0 +end + \ No newline at end of file From ff91c4cdfd62a3e42a510383cb029426c9fa35da Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Thu, 13 Apr 2017 15:15:02 +0200 Subject: [PATCH 06/17] atomized job##finished --- lib/job.js | 38 ++++----- lib/scripts.js | 156 ++++++++++++++++++++----------------- lib/scripts/isFinished.lua | 22 ++++++ test/test_job.js | 10 +-- 4 files changed, 124 insertions(+), 102 deletions(-) create mode 100644 lib/scripts/isFinished.lua diff --git a/lib/job.js b/lib/job.js index 32fbad788..d2dffce1f 100644 --- a/lib/job.js +++ b/lib/job.js @@ -7,6 +7,8 @@ var scripts = require('./scripts'); var debuglog = require('debuglog')('bull'); var errors = require('./errors'); +var FINISHED_WATCHDOG = 5000; + /** interface JobOptions { @@ -327,32 +329,20 @@ Job.prototype.remove = function(){ /** * Returns a promise the resolves when the job has been finished. - * TODO: Add a watchdog to check if the job has finished periodically. - * since pubsub does not give any guarantees. */ Job.prototype.finished = function(){ var _this = this; - function status(resolve, reject){ - return _this.isCompleted().then(function(completed){ - if(!completed){ - return _this.isFailed().then(function(failed){ - if(failed){ - return Job.fromId(_this.queue, _this.id, 'failedReason').then(function(data){ - reject(Error(data.failedReason)); - return true; - }); - } + return scripts.isFinished(_this).then(function(status){ + var finished = status > 0; + if(finished){ + if(status == 2){ + return Job.fromId(_this.queue, _this.id, 'failedReason').then(function(data){ + throw Error(data.failedReason); }); } - resolve(); - return true; - }); - } - - return new Promise(function(resolve, reject){ - status(resolve, reject).then(function(finished){ - if(!finished){ + }else{ + return new Promise(function(resolve, reject){ var interval; function onCompleted(job){ if(String(job.id) === String(_this.id)){ @@ -385,12 +375,12 @@ Job.prototype.finished = function(){ status(resolve, reject).then(function(finished){ if(finished){ removeListeners(); - clearInterval(interval ); + clearInterval(interval); } }) - }, 5000); - }; - }); + }, FINISHED_WATCHDOG); + }); + }; }); } diff --git a/lib/scripts.js b/lib/scripts.js index e9d39234a..030e2511d 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -53,6 +53,7 @@ var scripts = { return result === 1; }); }, + addJob: function(client, toKey, job, opts){ var queue = job.queue; var lua = loadScriptIfNeeded(client, 'addJob'); @@ -96,78 +97,6 @@ var scripts = { return runScript(queue.client, 'pause', lua, keys, [pause ? 'paused' : 'resumed']) }, - // - // TODO: DEPRECATE. - // - move: function(job, src, target){ - // TODO: Depending on the source we should use LREM or ZREM. - // TODO: Depending on the target we should use LPUSH, SADD, etc. - var keys = _.map([ - src, - target, - job.id - ], function(name){ - return job.queue.toKey(name); - } - ); - - var deleteJob = 'redis.call("DEL", KEYS[3])'; - - var moveJob = [ - 'redis.call("ZADD", KEYS[2], ARGV[3], ARGV[1])', - 'redis.call("HSET", KEYS[3], "returnvalue", ARGV[2])', - ].join('\n'); - - var script = [ - 'if redis.call("EXISTS", KEYS[3]) == 1 then', // Make sure job exists - ' redis.call("LREM", KEYS[1], -1, ARGV[1])', - target ? moveJob : deleteJob, - ' return 0', - 'else', - ' return -1', - 'end' - ].join('\n'); - - var args = [ - job.queue.client, - 'move' + src + (target ? target : ''), - script, - keys.length, - keys[0], - keys[1], - keys[2], - job.id, - job.returnvalue ? JSON.stringify(job.returnvalue) : '', - parseInt(job.timestamp) - ]; - - var returnLockOrErrorCode = function(lock) { - return lock ? execScript.apply(scripts, args) : -2; - }; - var throwUnexpectedErrors = function(err) { - if (!(err instanceof Redlock.LockError)) { - throw err; - } - }; - - return job.takeLock(!!job.lock) - .then(returnLockOrErrorCode, throwUnexpectedErrors) - .then(function(result){ - switch (result){ - case -1: - if(src){ - throw new Error('Missing Job ' + job.id + ' when trying to move from ' + src + ' to ' + target); - } else { - throw new Error('Missing Job ' + job.id + ' when trying to remove it from ' + src); - } - case -2: - throw new Error('Cannot get lock for job ' + job.id + ' when trying to move from ' + src); - default: - return job.releaseLock(); - } - }); - }, - moveToActive: function(queue){ var lua = loadScriptIfNeeded(queue.client, 'moveToActive'); @@ -224,6 +153,15 @@ var scripts = { return scripts.moveToFinished(job, failedReason, 'failedReason', removeOnFailed, 'failed', ignoreLock); }, + isFinished: function(job){ + var lua = loadScriptIfNeeded(job.queue.client, 'isFinished'); + var keys = _.map(['completed', 'failed'], function(key){ + return job.queue.toKey(key); + }); + + return runScript(job.queue.client, 'isFinished', lua, keys, [job.id]); + }, + moveToSet: function(queue, set, jobId, context){ var lua = loadScriptIfNeeded(queue.client, 'moveToSet'); @@ -334,7 +272,7 @@ var scripts = { return runScript(queue.client, 'moveUnlockedJobsToWait', lua, keys, args); }, - // TODO: Refacto into lua script + // TODO: Refactor into lua script cleanJobsInSet: function(queue, set, ts, limit) { var command; var removeCommand; @@ -456,6 +394,78 @@ var scripts = { ]; return runScript(queue.client, 'reprocessJob', lua, keys, args); + }, + + // + // TODO: DEPRECATE. + // + move: function(job, src, target){ + // TODO: Depending on the source we should use LREM or ZREM. + // TODO: Depending on the target we should use LPUSH, SADD, etc. + var keys = _.map([ + src, + target, + job.id + ], function(name){ + return job.queue.toKey(name); + } + ); + + var deleteJob = 'redis.call("DEL", KEYS[3])'; + + var moveJob = [ + 'redis.call("ZADD", KEYS[2], ARGV[3], ARGV[1])', + 'redis.call("HSET", KEYS[3], "returnvalue", ARGV[2])', + ].join('\n'); + + var script = [ + 'if redis.call("EXISTS", KEYS[3]) == 1 then', // Make sure job exists + ' redis.call("LREM", KEYS[1], -1, ARGV[1])', + target ? moveJob : deleteJob, + ' return 0', + 'else', + ' return -1', + 'end' + ].join('\n'); + + var args = [ + job.queue.client, + 'move' + src + (target ? target : ''), + script, + keys.length, + keys[0], + keys[1], + keys[2], + job.id, + job.returnvalue ? JSON.stringify(job.returnvalue) : '', + parseInt(job.timestamp) + ]; + + var returnLockOrErrorCode = function(lock) { + return lock ? execScript.apply(scripts, args) : -2; + }; + var throwUnexpectedErrors = function(err) { + if (!(err instanceof Redlock.LockError)) { + throw err; + } + }; + + return job.takeLock(!!job.lock) + .then(returnLockOrErrorCode, throwUnexpectedErrors) + .then(function(result){ + switch (result){ + case -1: + if(src){ + throw new Error('Missing Job ' + job.id + ' when trying to move from ' + src + ' to ' + target); + } else { + throw new Error('Missing Job ' + job.id + ' when trying to remove it from ' + src); + } + case -2: + throw new Error('Cannot get lock for job ' + job.id + ' when trying to move from ' + src); + default: + return job.releaseLock(); + } + }); } }; diff --git a/lib/scripts/isFinished.lua b/lib/scripts/isFinished.lua new file mode 100644 index 000000000..3b1066d5c --- /dev/null +++ b/lib/scripts/isFinished.lua @@ -0,0 +1,22 @@ +--[[ + Checks if a job is finished (.i.e. is in the completed or failed set) + + Input: + KEYS[1] completed key + KEYS[2] failed key + + ARGV[1] job id + Output: + 0 - not finished. + 1 - completed. + 2 - failed. +]] +if redis.call("ZSCORE", KEYS[1], ARGV[1]) ~= false then + return 1 +end + +if redis.call("ZSCORE", KEYS[2], ARGV[1]) ~= false then + return 2 +end + +return redis.call("ZSCORE", KEYS[2], ARGV[1]) diff --git a/test/test_job.js b/test/test_job.js index 46d2c446c..afe40ed80 100644 --- a/test/test_job.js +++ b/test/test_job.js @@ -451,18 +451,18 @@ describe('Job', function(){ describe('.finished', function() { it('should resolve when the job has been completed', function(done){ queue.process(function () { - return Promise.resolve(); + return Promise.delay(500); }); queue.add({ foo: 'bar' }).then(function(job){ return job.finished(); - }).then(function(){ - done(); - }, done); + }).then(done, done); }); it('should reject when the job has been completed', function(done){ queue.process(function () { - return Promise.reject(Error('test error')); + return Promise.delay(500).then(function(){ + return Promise.reject(Error('test error')); + }); }); queue.add({ foo: 'bar' }).then(function(job){ return job.finished(); From e565ed627b05fdd0fc27f76ff9fcc3040e1fc306 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Thu, 13 Apr 2017 15:23:29 +0200 Subject: [PATCH 07/17] processing listeners cleanup --- lib/queue.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/queue.js b/lib/queue.js index fbb5d1bb3..3c9b05abc 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -766,15 +766,26 @@ Queue.prototype.getNextJob = function() { // // Listen for new jobs, during moveToActive or after. // + var resolver; var newJobs = new Promise(function(resolve){ - resolve = resolve.bind(resolve, void 0) - _this.once('added', resolve); - _this.once('resumed', resolve); - _this.once('wait-finished', resolve); + resolver = function(){ + removeListeners(); + resolve(); + } + _this.on('added', resolver); + _this.on('resumed', resolver); + _this.on('wait-finished', resolver); }); + var removeListeners = function(){ + _this.removeListener('added', resolver); + _this.removeListener('resumed', resolver); + _this.removeListener('wait-finished', resolver); + } + return scripts.moveToActive(this).then(function(jobData){ if(jobData){ + removeListeners(); return Job.fromData(_this, jobData); }else{ return newJobs; From a6f7fc5f09d4008dd32fe0d6d4e8ed8808f41044 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Thu, 13 Apr 2017 15:23:39 +0200 Subject: [PATCH 08/17] increased timeout --- test/test_queue.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_queue.js b/test/test_queue.js index 1bf819bab..eeca9e307 100644 --- a/test/test_queue.js +++ b/test/test_queue.js @@ -585,7 +585,7 @@ describe('Queue', function () { it('process stalled jobs when starting a queue', function (done) { - this.timeout(2000); + this.timeout(6000); utils.newQueue('test queue stalled').then(function (queueStalled) { queueStalled.LOCK_DURATION = 15; queueStalled.LOCK_RENEW_TIME = 5 From 55da1e5de6e8ebc82e978e8caec6ec8d22652411 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sat, 15 Apr 2017 23:41:57 +0200 Subject: [PATCH 09/17] set jobid and wait event to waiting --- lib/job.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/job.js b/lib/job.js index d2dffce1f..c745ece41 100644 --- a/lib/job.js +++ b/lib/job.js @@ -57,7 +57,7 @@ Job.create = function(queue, name, data, opts){ return addJob(queue, job).then(function(jobId){ job.id = jobId; - queue.distEmit('waiting', job); + queue.distEmit('waiting', job, null); debuglog('Job added', jobId); return job; }); @@ -70,7 +70,7 @@ Job.fromId = function(queue, jobId){ } return queue.client.hgetall(queue.toKey(jobId)).then(function(jobData){ if(!_.isEmpty(jobData)){ - return Job.fromData(queue, jobData); + return Job.fromData(queue, jobData, jobId); }else{ return null; } @@ -91,8 +91,8 @@ Job.prototype.progress = function(progress){ Job.prototype.toJSON = function(){ var opts = _.extend({}, this.opts || {}); - opts.jobId = this.id; return { + id: this.id, name: this.name, data: this.data || {}, opts: opts, @@ -112,7 +112,6 @@ Job.prototype.toData = function(){ json.opts = JSON.stringify(json.opts); json.stacktrace = JSON.stringify(json.stacktrace); json.returnvalue = JSON.stringify(json.returnvalue); - return json; }; @@ -250,7 +249,7 @@ Job.prototype.retry = function(){ var _this = this; return scripts.reprocessJob(this, { state: 'failed' }).then(function(result) { if (result === 1) { - queue.emit('waiting', _this); + queue.distEmit('waiting', _this, null); } else if (result === 0) { throw new Error(errors.Messages.RETRY_JOB_NOT_EXIST); } else if (result === -1) { @@ -435,20 +434,20 @@ Job.prototype._saveAttempt = function(err){ return this.queue.client.hmset(this.queue.toKey(this.id), params); }; -Job.fromData = function(queue, raw){ +Job.fromData = function(queue, raw, jobId){ raw = _.extend({}, raw); raw.data = JSON.parse(raw.data); raw.opts = JSON.parse(raw.opts); - var job = Job.fromJSON(queue, raw); + var job = Job.fromJSON(queue, raw, jobId); return job; }; -Job.fromJSON = function(queue, json){ +Job.fromJSON = function(queue, json, jobId){ var job = new Job(queue, json.name || Job.DEFAULT_JOB_NAME, json.data, json.opts); - job.id = json.id; + job.id = json.id || jobId; job._progress = parseInt(json.progress || 0); job.delay = parseInt(json.delay); job.timestamp = parseInt(json.timestamp); From 1d26d434456eea20af20e7b989d6e59bb5b98374 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sat, 15 Apr 2017 23:43:34 +0200 Subject: [PATCH 10/17] start end in getJobs. Events with from status --- lib/queue.js | 43 +++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/lib/queue.js b/lib/queue.js index 3c9b05abc..d596cac24 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -228,7 +228,6 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ listenDistEvent('active'); // listenDistEvent('progress'); // listenDistEvent('stalled'); // - listenDistEvent('active'); // listenDistEvent('completed'); // listenDistEvent('failed'); // listenDistEvent('cleaned'); @@ -624,7 +623,7 @@ Queue.prototype.moveUnlockedJobsToWait = function(){ return scripts.moveUnlockedJobsToWait(this).then(function(responses){ var handleFailedJobs = responses[0].map(function(jobId){ return _this.getJobFromId(jobId).then(function(job){ - _this.distEmit('failed', job, new Error('job stalled more than allowable limit')); + _this.distEmit('failed', job, new Error('job stalled more than allowable limit'), 'active' ); return null; }); }); @@ -672,7 +671,7 @@ Queue.prototype.processJobs = function(index, resolve, reject){ Queue.prototype.processJob = function(job){ var _this = this; var lockRenewId; - var timmerStopped = false; + var timerStopped = false; if(!job){ return Promise.resolve(); @@ -689,7 +688,7 @@ Queue.prototype.processJob = function(job){ // var lockExtender = function(){ _this.timers.set('lockExtender', _this.LOCK_RENEW_TIME, function(){ - if(!timmerStopped){ + if(!timerStopped){ scripts.extendLock(_this, job.id).then(function(lock){ if(lock){ lockExtender(); @@ -702,7 +701,7 @@ Queue.prototype.processJob = function(job){ var timeoutMs = job.opts.timeout; function stopTimer(){ - timmerStopped = true; + timerStopped = true; _this.timers.clear(lockRenewId); } @@ -716,7 +715,7 @@ Queue.prototype.processJob = function(job){ stopTimer(); return job.moveToCompleted(result).then(function(){ - return _this.distEmit('completed', job, result); + return _this.distEmit('completed', job, result, 'active'); }); } @@ -728,7 +727,7 @@ Queue.prototype.processJob = function(job){ // See https://github.com/OptimalBits/bull/pull/415#issuecomment-269744735 // job.takeLock(true /* renew */, false /* ensureActive */).then( function(/*lock*/) { return job.moveToFailed(err).then(function(){ - return _this.distEmit('failed', job, error); + return _this.distEmit('failed', job, error, 'active'); }); } @@ -743,7 +742,7 @@ Queue.prototype.processJob = function(job){ jobPromise = jobPromise.timeout(timeoutMs); } - _this.distEmit('active', job, jobPromise); + _this.distEmit('active', job, jobPromise, 'waiting'); return jobPromise.then(handleCompleted, handleFailed); } @@ -783,10 +782,10 @@ Queue.prototype.getNextJob = function() { _this.removeListener('wait-finished', resolver); } - return scripts.moveToActive(this).then(function(jobData){ + return scripts.moveToActive(this).spread(function(jobData, jobId){ if(jobData){ removeListeners(); - return Job.fromData(_this, jobData); + return Job.fromData(_this, jobData, jobId); }else{ return newJobs; } @@ -839,7 +838,7 @@ Queue.prototype.getJobCountByTypes = function() { * */ Queue.prototype.getJobCounts = function(){ - var types = ['wait', 'active', 'completed', 'failed', 'delayed']; + var types = ['waiting', 'active', 'completed', 'failed', 'delayed']; var counts = {}; return this.client.multi() .llen(this.toKey('wait')) @@ -879,28 +878,28 @@ Queue.prototype.getPausedCount = function() { return this.client.llen(this.toKey('paused')); }; -Queue.prototype.getWaiting = function(/*start, end*/){ +Queue.prototype.getWaiting = function(start, end){ return Promise.join( - this.getJobs('wait', 'LIST'), - this.getJobs('paused', 'LIST')).spread(function(waiting, paused){ + this.getJobs('wait', 'LIST', start, end), + this.getJobs('paused', 'LIST', start, end)).spread(function(waiting, paused){ return _.concat(waiting, paused); }); }; -Queue.prototype.getActive = function(/*start, end*/){ - return this.getJobs('active', 'LIST'); +Queue.prototype.getActive = function(start, end){ + return this.getJobs('active', 'LIST', start, end); }; -Queue.prototype.getDelayed = function(/*start, end*/){ - return this.getJobs('delayed', 'ZSET'); +Queue.prototype.getDelayed = function(start, end){ + return this.getJobs('delayed', 'ZSET', start, end); }; -Queue.prototype.getCompleted = function(){ - return this.getJobs('completed', 'ZSET'); +Queue.prototype.getCompleted = function(start, end){ + return this.getJobs('completed', 'ZSET', start, end); }; -Queue.prototype.getFailed = function(){ - return this.getJobs('failed', 'ZSET'); +Queue.prototype.getFailed = function(start, end){ + return this.getJobs('failed', 'ZSET', start, end); }; Queue.prototype.getJobs = function(queueType, type, start, end){ From 8473c095abb415cb409956c7983f25b1f150a6cc Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sat, 15 Apr 2017 23:43:53 +0200 Subject: [PATCH 11/17] return jobId when moving to active --- lib/scripts.js | 13 +++++++++---- lib/scripts/addJob.lua | 2 +- lib/scripts/moveToActive.lua | 11 ++--------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/scripts.js b/lib/scripts.js index 030e2511d..6bbeaec58 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -114,10 +114,15 @@ var scripts = { queue.LOCK_DURATION ]; - return runScript(queue.client, 'moveToActive', lua, keys, args).then(function(jobData){ - if(jobData && jobData.length){ - return array2hash(jobData); + return runScript(queue.client, 'moveToActive', lua, keys, args).then(function(result){ + if(result){ + var jobData = result[0]; + if(jobData.length){ + var job = array2obj(jobData); + return [job, result[1]]; + } } + return []; }); }, @@ -471,7 +476,7 @@ var scripts = { module.exports = scripts; -function array2hash(arr){ +function array2obj(arr){ var obj = {}; for(var i=0; i < arr.length; i+=2){ obj[arr[i]] = arr[i+1] diff --git a/lib/scripts/addJob.lua b/lib/scripts/addJob.lua index b018ce84a..05a89fe1c 100644 --- a/lib/scripts/addJob.lua +++ b/lib/scripts/addJob.lua @@ -51,7 +51,7 @@ if redis.call("EXISTS", jobIdKey) == 1 then end -- Store the job. -redis.call("HMSET", jobIdKey,"id", jobId, "name", ARGV[3], "data", ARGV[4], "opts", ARGV[5], "timestamp", ARGV[6], "delay", ARGV[7]) +redis.call("HMSET", jobIdKey, "name", ARGV[3], "data", ARGV[4], "opts", ARGV[5], "timestamp", ARGV[6], "delay", ARGV[7]) -- Check if job is delayed if(ARGV[8] ~= "0") then diff --git a/lib/scripts/moveToActive.lua b/lib/scripts/moveToActive.lua index 8530d28ee..1cd986fe0 100644 --- a/lib/scripts/moveToActive.lua +++ b/lib/scripts/moveToActive.lua @@ -29,14 +29,7 @@ if jobId then redis.call("LREM", KEYS[1], 1, jobId) -- remove from wait redis.call("ZREM", KEYS[3], jobId) -- remove from priority redis.call("LPUSH", KEYS[2], jobId) -- push in active - return redis.call("HGETALL", jobKey) -- get job data + + return {redis.call("HGETALL", jobKey), jobId} -- get job data end ---[[ - Release lock: - if redis.call("GET", KEYS[1]) == ARGV[1] then - return redis.call("del", KEYS[1]) - else - return 0 - end -]] From e8e55bba4fd621b6839e6e32ad1f17bb75f48a78 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sun, 16 Apr 2017 01:21:32 +0200 Subject: [PATCH 12/17] changelog updated --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42533b1a0..b44e9d4b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,9 @@ v.3.0.0-alpha ============= - +- job.jobId changed to job.id. +- refactored error messages into separate error module. +- completed and failed job states are now represented in ZSETs. v.2.2.6 ======= From f3b758597de7a60d04c3838dbd98dbefb32edd98 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sun, 16 Apr 2017 01:23:59 +0200 Subject: [PATCH 13/17] removed deprecated code. Moved stopTimer to finally. --- lib/job.js | 31 +----------- lib/queue.js | 16 +++--- lib/scripts.js | 92 ++++------------------------------ lib/scripts/moveToDelayed.lua | 32 ++++++++++++ lib/scripts/moveToFinished.lua | 2 +- lib/scripts/moveToSet.lua | 42 ---------------- test/test_job.js | 5 +- 7 files changed, 53 insertions(+), 167 deletions(-) create mode 100644 lib/scripts/moveToDelayed.lua delete mode 100644 lib/scripts/moveToSet.lua diff --git a/lib/job.js b/lib/job.js index c745ece41..aa2dc1d2e 100644 --- a/lib/job.js +++ b/lib/job.js @@ -151,33 +151,11 @@ Job.prototype.releaseLock = function(){ }); }; -Job.prototype.delayIfNeeded = function(){ - if(this.delay){ - var jobDelayedTimestamp = this.timestamp + this.delay; - if(jobDelayedTimestamp > Date.now()){ - return this.moveToDelayed(jobDelayedTimestamp).then(function(){ - return true; - }); - } - } - return Promise.resolve(false); -}; - Job.prototype.moveToCompleted = function(returnValue, ignoreLock){ this.returnvalue = returnValue || 0; return scripts.moveToCompleted(this, this.returnvalue, this.opts.removeOnComplete, ignoreLock); }; -Job.prototype.move = function(src, target, returnValue){ - if(target === 'completed'){ - this.returnvalue = returnValue || 0; - if(this.opts.removeOnComplete){ - target = void 0; - } - } - return scripts.move(this, src, target); -} - Job.prototype.discard = function(){ this._discarded = true; } @@ -205,7 +183,7 @@ Job.prototype.moveToFailed = function(err, ignoreLock){ }; Job.prototype.moveToDelayed = function(timestamp){ - return this._moveToSet('delayed', timestamp); + return scripts.moveToDelayed(this.queue, this.id, timestamp); }; Job.prototype.promote = function(){ @@ -397,13 +375,6 @@ Job.prototype._isInList = function(list) { return scripts.isJobInList(this.queue.client, this.queue.toKey(list), this.id); }; -Job.prototype._moveToSet = function(set, context){ - var queue = this.queue; - var jobId = this.id; - - return scripts.moveToSet(queue, set, jobId, context); -}; - Job.prototype._getBackOff = function() { var backoff = 0; var delay; diff --git a/lib/queue.js b/lib/queue.js index d596cac24..e3e0f1eda 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -291,10 +291,6 @@ Queue.prototype.isReady = function(){ }); } -Queue.prototype.getJobMoveCount = function(){ - return this.client.commandQueue.length; -}; - Queue.prototype.whenCurrentMoveFinished = function(){ var currentMove = this.client.commandQueue.peekFront() return currentMove && currentMove.command.promise || Promise.resolve(); @@ -712,23 +708,23 @@ Queue.prototype.processJob = function(job){ return handleFailed(err); } - stopTimer(); - return job.moveToCompleted(result).then(function(){ return _this.distEmit('completed', job, result, 'active'); - }); + }).finally(function(){ + stopTimer(); + }) } function handleFailed(err){ var error = err.cause || err; //Handle explicit rejection - stopTimer(); - // See https://github.com/OptimalBits/bull/pull/415#issuecomment-269744735 // job.takeLock(true /* renew */, false /* ensureActive */).then( function(/*lock*/) { return job.moveToFailed(err).then(function(){ return _this.distEmit('failed', job, error, 'active'); - }); + }).finally(function(){ + stopTimer(); + }) } lockExtender() diff --git a/lib/scripts.js b/lib/scripts.js index 6bbeaec58..c3f40eb6a 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -167,8 +167,8 @@ var scripts = { return runScript(job.queue.client, 'isFinished', lua, keys, [job.id]); }, - moveToSet: function(queue, set, jobId, context){ - var lua = loadScriptIfNeeded(queue.client, 'moveToSet'); + moveToDelayed: function(queue, jobId, context){ + var lua = loadScriptIfNeeded(queue.client, 'moveToDelayed'); // // Bake in the job id first 12 bits into the timestamp @@ -179,26 +179,24 @@ var scripts = { // context = _.isUndefined(context) ? 0 : context; - if(set === 'delayed') { - context = +context || 0; - context = context < 0 ? 0 : context; - if(context > 0){ - context = context * 0x1000 + (jobId & 0xfff); - } + context = +context || 0; + context = context < 0 ? 0 : context; + if(context > 0){ + context = context * 0x1000 + (jobId & 0xfff); } var keys = _.map([ 'active', - set, + 'delayed', jobId ], function(name){ return queue.toKey(name); } ); - return runScript(queue.client, 'moveToSet', lua, keys, [JSON.stringify(context), jobId]).then(function(result){ + return runScript(queue.client, 'moveToDelayed', lua, keys, [JSON.stringify(context), jobId]).then(function(result){ if(result === -1){ - throw new Error('Missing Job ' + jobId + ' when trying to move from active to ' + set); + throw new Error('Missing Job ' + jobId + ' when trying to move from active to delayed'); } }); }, @@ -399,78 +397,6 @@ var scripts = { ]; return runScript(queue.client, 'reprocessJob', lua, keys, args); - }, - - // - // TODO: DEPRECATE. - // - move: function(job, src, target){ - // TODO: Depending on the source we should use LREM or ZREM. - // TODO: Depending on the target we should use LPUSH, SADD, etc. - var keys = _.map([ - src, - target, - job.id - ], function(name){ - return job.queue.toKey(name); - } - ); - - var deleteJob = 'redis.call("DEL", KEYS[3])'; - - var moveJob = [ - 'redis.call("ZADD", KEYS[2], ARGV[3], ARGV[1])', - 'redis.call("HSET", KEYS[3], "returnvalue", ARGV[2])', - ].join('\n'); - - var script = [ - 'if redis.call("EXISTS", KEYS[3]) == 1 then', // Make sure job exists - ' redis.call("LREM", KEYS[1], -1, ARGV[1])', - target ? moveJob : deleteJob, - ' return 0', - 'else', - ' return -1', - 'end' - ].join('\n'); - - var args = [ - job.queue.client, - 'move' + src + (target ? target : ''), - script, - keys.length, - keys[0], - keys[1], - keys[2], - job.id, - job.returnvalue ? JSON.stringify(job.returnvalue) : '', - parseInt(job.timestamp) - ]; - - var returnLockOrErrorCode = function(lock) { - return lock ? execScript.apply(scripts, args) : -2; - }; - var throwUnexpectedErrors = function(err) { - if (!(err instanceof Redlock.LockError)) { - throw err; - } - }; - - return job.takeLock(!!job.lock) - .then(returnLockOrErrorCode, throwUnexpectedErrors) - .then(function(result){ - switch (result){ - case -1: - if(src){ - throw new Error('Missing Job ' + job.id + ' when trying to move from ' + src + ' to ' + target); - } else { - throw new Error('Missing Job ' + job.id + ' when trying to remove it from ' + src); - } - case -2: - throw new Error('Cannot get lock for job ' + job.id + ' when trying to move from ' + src); - default: - return job.releaseLock(); - } - }); } }; diff --git a/lib/scripts/moveToDelayed.lua b/lib/scripts/moveToDelayed.lua new file mode 100644 index 000000000..2eb3f5176 --- /dev/null +++ b/lib/scripts/moveToDelayed.lua @@ -0,0 +1,32 @@ +--[[ + Moves job from active to delayed set. + + Input: + KEYS[1] active key + KEYS[2] delayed key + KEYS[3] job key + + ARGV[1] delayedTimestamp + ARGV[2] the id of the job + ARGV[3] timestamp + + Output: + 0 - OK + -1 - Missing job. + + Events: + - delayed key. +]] +if redis.call("EXISTS", KEYS[3]) == 1 then + local score = tonumber(ARGV[1]) + if score ~= 0 then + redis.call("ZADD", KEYS[2], score, ARGV[2]) + redis.call("PUBLISH", KEYS[2], (score / 0x1000)) + else + redis.call("ZADD", KEYS[2], ARGV[3], ARGV[2]) + end + redis.call("LREM", KEYS[1], 0, ARGV[2]) + return 0; +else + return -1 +end diff --git a/lib/scripts/moveToFinished.lua b/lib/scripts/moveToFinished.lua index 58b8b7bf9..3f17f7450 100644 --- a/lib/scripts/moveToFinished.lua +++ b/lib/scripts/moveToFinished.lua @@ -20,7 +20,7 @@ Output: 0 OK - 1 Missing key. + -1 Missing key. Events: 'completed' diff --git a/lib/scripts/moveToSet.lua b/lib/scripts/moveToSet.lua deleted file mode 100644 index 6b962843a..000000000 --- a/lib/scripts/moveToSet.lua +++ /dev/null @@ -1,42 +0,0 @@ ---[[this lua script takes three keys and two arguments - // keys: - // - the expanded key for the active set - // - the expanded key for the destination set - // - the expanded key for the job - // - // arguments: - // ARGV[1] json serialized context which is: - // - delayedTimestamp when the destination set is 'delayed' - // - stacktrace when the destination set is 'failed' - // - returnvalue of the handler when the destination set is 'completed' - // ARGV[2] the id of the job - // ARGV[3] timestamp - // - // it checks whether KEYS[2] the destination set ends with 'delayed', 'completed' - // or 'failed'. And then adds the context to the jobhash and adds the job to - // the destination set. Finally it removes the job from the active list. - // - // it returns either 0 for success or -1 for failure. -]] -if redis.call("EXISTS", KEYS[3]) == 1 then - if string.find(KEYS[2], "delayed$") ~= nil then - local score = tonumber(ARGV[1]) - if score ~= 0 then - redis.call("ZADD", KEYS[2], score, ARGV[2]) - redis.call("PUBLISH", KEYS[2], (score / 0x1000)) - else - redis.call("ZADD", KEYS[2], ARGV[3], ARGV[2]) - end - elseif string.find(KEYS[2], "completed$") ~= nil then - redis.call("HSET", KEYS[3], "returnvalue", ARGV[1]) - redis.call("ZADD", KEYS[2], ARGV[3], ARGV[2]) - elseif string.find(KEYS[2], "failed$") ~= nil then - redis.call("ZADD", KEYS[2], ARGV[3], ARGV[2]) - else - return -1 - end - redis.call("LREM", KEYS[1], 0, ARGV[2]) - return 0 - else - return -1 -end diff --git a/test/test_job.js b/test/test_job.js index afe40ed80..8d7ba57e3 100644 --- a/test/test_job.js +++ b/test/test_job.js @@ -380,6 +380,7 @@ describe('Job', function(){ // TODO: // Divide into several tests // + var scripts = require('../lib/scripts'); it('get job status', function() { this.timeout(12000); @@ -390,7 +391,9 @@ describe('Job', function(){ return job.getState(); }).then(function(state) { expect(state).to.be('waiting'); - return job.move('wait', 'completed'); + return scripts.moveToActive(queue).then(function(){ + return job.moveToCompleted(); + }); }).then(function (){ return job.isCompleted(); }).then(function (isCompleted) { From 19bfe1b094b1ea9b64b3750abe0d8bfe69a1dc60 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sun, 16 Apr 2017 12:16:16 +0200 Subject: [PATCH 14/17] removed obsolete comments --- lib/job.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/job.js b/lib/job.js index aa2dc1d2e..f6192c237 100644 --- a/lib/job.js +++ b/lib/job.js @@ -14,10 +14,10 @@ interface JobOptions { priority: Priority; attempts: number; + delay: number; } */ - // queue: Queue, data: {}, opts: JobOptions var Job = function(queue, name, data, opts){ if(typeof name !== 'string'){ @@ -160,7 +160,10 @@ Job.prototype.discard = function(){ this._discarded = true; } -// This needs to be atomized in a LUA script. +// +// This could be atomized using "multi", since performance is not so critical in the failed +// case. For this to work we need to be able to define the lua commands at start up time. +// Job.prototype.moveToFailed = function(err, ignoreLock){ var _this = this; return this._saveAttempt(err).then(function() { @@ -305,7 +308,7 @@ Job.prototype.remove = function(){ }; /** - * Returns a promise the resolves when the job has been finished. + * Returns a promise the resolves when the job has finished. (completed or failed). */ Job.prototype.finished = function(){ var _this = this; From 101019f4a3fc9e9831c140b1dd005ba732ba53ff Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sun, 16 Apr 2017 18:28:58 +0200 Subject: [PATCH 15/17] preload lua scripts --- .../addJob.lua => commands/addJob-7.lua} | 0 .../extendLock-1.lua} | 0 lib/commands/index.js | 45 +++++++ .../isFinished-2.lua} | 0 .../isJobInList-1.lua} | 0 .../moveToActive-3.lua} | 0 .../moveToDelayed-3.lua} | 0 .../moveToFinished-3.lua} | 0 .../moveUnlockedJobsToWait-4.lua} | 0 .../pause.lua => commands/pause-4.lua} | 1 - .../releaseLock-1.lua} | 0 .../removeJob-7.lua} | 0 .../reprocessJob-5.lua} | 0 .../retryJob.lua => commands/retryJob-4.lua} | 0 .../takeLock.lua => commands/takeLock-1.lua} | 0 .../updateDelaySet-4.lua} | 0 lib/queue.js | 36 +++--- lib/scripts.js | 117 +++++++----------- lib/scripts/index.js | 36 ------ 19 files changed, 106 insertions(+), 129 deletions(-) rename lib/{scripts/addJob.lua => commands/addJob-7.lua} (100%) rename lib/{scripts/extendLock.lua => commands/extendLock-1.lua} (100%) create mode 100644 lib/commands/index.js rename lib/{scripts/isFinished.lua => commands/isFinished-2.lua} (100%) rename lib/{scripts/isJobInList.lua => commands/isJobInList-1.lua} (100%) rename lib/{scripts/moveToActive.lua => commands/moveToActive-3.lua} (100%) rename lib/{scripts/moveToDelayed.lua => commands/moveToDelayed-3.lua} (100%) rename lib/{scripts/moveToFinished.lua => commands/moveToFinished-3.lua} (100%) rename lib/{scripts/moveUnlockedJobsToWait.lua => commands/moveUnlockedJobsToWait-4.lua} (100%) rename lib/{scripts/pause.lua => commands/pause-4.lua} (99%) rename lib/{scripts/releaseLock.lua => commands/releaseLock-1.lua} (100%) rename lib/{scripts/removeJob.lua => commands/removeJob-7.lua} (100%) rename lib/{scripts/reprocessJob.lua => commands/reprocessJob-5.lua} (100%) rename lib/{scripts/retryJob.lua => commands/retryJob-4.lua} (100%) rename lib/{scripts/takeLock.lua => commands/takeLock-1.lua} (100%) rename lib/{scripts/updateDelaySet.lua => commands/updateDelaySet-4.lua} (100%) delete mode 100644 lib/scripts/index.js diff --git a/lib/scripts/addJob.lua b/lib/commands/addJob-7.lua similarity index 100% rename from lib/scripts/addJob.lua rename to lib/commands/addJob-7.lua diff --git a/lib/scripts/extendLock.lua b/lib/commands/extendLock-1.lua similarity index 100% rename from lib/scripts/extendLock.lua rename to lib/commands/extendLock-1.lua diff --git a/lib/commands/index.js b/lib/commands/index.js new file mode 100644 index 000000000..09365e85b --- /dev/null +++ b/lib/commands/index.js @@ -0,0 +1,45 @@ +/** + * Load redis lua scripts. + * The name of the script must have the following format: + * + * cmdName-numKeys.lua + * + * cmdName must be in camel case format. + * + * For example: + * moveToFinish-3.lua + * + */ +'use strict'; + +var fs = require('fs'); +var path = require('path'); +var Promise = require('bluebird'); + +fs = Promise.promisifyAll(fs); + +// +// for some very strange reason, defining scripts with this code results in this error +// when executing the scripts: ERR value is not an integer or out of range +// +module.exports = function(client){ + return loadScripts(client, __dirname); +} + +function loadScripts(client, dir) { + return fs.readdirAsync(dir).then(function(files){ + return Promise.all(files.filter(function (file) { + return path.extname(file) === '.lua'; + }).map(function (file) { + var longName = path.basename(file, '.lua'); + var name = longName.split('-')[0]; + var numberOfKeys = parseInt(longName.split('-')[1]); + + return fs.readFileAsync(path.join(dir, file)).then(function(lua){ + client.defineCommand(name, { numberOfKeys: numberOfKeys, lua: lua.toString() }); + }, function(err){ + console.log('Error reading script file', err) + }); + })); + }); +}; diff --git a/lib/scripts/isFinished.lua b/lib/commands/isFinished-2.lua similarity index 100% rename from lib/scripts/isFinished.lua rename to lib/commands/isFinished-2.lua diff --git a/lib/scripts/isJobInList.lua b/lib/commands/isJobInList-1.lua similarity index 100% rename from lib/scripts/isJobInList.lua rename to lib/commands/isJobInList-1.lua diff --git a/lib/scripts/moveToActive.lua b/lib/commands/moveToActive-3.lua similarity index 100% rename from lib/scripts/moveToActive.lua rename to lib/commands/moveToActive-3.lua diff --git a/lib/scripts/moveToDelayed.lua b/lib/commands/moveToDelayed-3.lua similarity index 100% rename from lib/scripts/moveToDelayed.lua rename to lib/commands/moveToDelayed-3.lua diff --git a/lib/scripts/moveToFinished.lua b/lib/commands/moveToFinished-3.lua similarity index 100% rename from lib/scripts/moveToFinished.lua rename to lib/commands/moveToFinished-3.lua diff --git a/lib/scripts/moveUnlockedJobsToWait.lua b/lib/commands/moveUnlockedJobsToWait-4.lua similarity index 100% rename from lib/scripts/moveUnlockedJobsToWait.lua rename to lib/commands/moveUnlockedJobsToWait-4.lua diff --git a/lib/scripts/pause.lua b/lib/commands/pause-4.lua similarity index 99% rename from lib/scripts/pause.lua rename to lib/commands/pause-4.lua index 090c3fb6e..ef7aa7b3d 100644 --- a/lib/scripts/pause.lua +++ b/lib/commands/pause-4.lua @@ -4,7 +4,6 @@ Note: This code is unnecessary complex, since it was used when we used BRPOPLPUSH. Currently only a meta-paused key is necessary. - ]] if redis.call("EXISTS", KEYS[1]) == 1 then redis.call("RENAME", KEYS[1], KEYS[2]) diff --git a/lib/scripts/releaseLock.lua b/lib/commands/releaseLock-1.lua similarity index 100% rename from lib/scripts/releaseLock.lua rename to lib/commands/releaseLock-1.lua diff --git a/lib/scripts/removeJob.lua b/lib/commands/removeJob-7.lua similarity index 100% rename from lib/scripts/removeJob.lua rename to lib/commands/removeJob-7.lua diff --git a/lib/scripts/reprocessJob.lua b/lib/commands/reprocessJob-5.lua similarity index 100% rename from lib/scripts/reprocessJob.lua rename to lib/commands/reprocessJob-5.lua diff --git a/lib/scripts/retryJob.lua b/lib/commands/retryJob-4.lua similarity index 100% rename from lib/scripts/retryJob.lua rename to lib/commands/retryJob-4.lua diff --git a/lib/scripts/takeLock.lua b/lib/commands/takeLock-1.lua similarity index 100% rename from lib/scripts/takeLock.lua rename to lib/commands/takeLock-1.lua diff --git a/lib/scripts/updateDelaySet.lua b/lib/commands/updateDelaySet-4.lua similarity index 100% rename from lib/scripts/updateDelaySet.lua rename to lib/commands/updateDelaySet-4.lua diff --git a/lib/queue.js b/lib/queue.js index e3e0f1eda..323f12e7a 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -17,6 +17,8 @@ var semver = require('semver'); var debuglog = require('debuglog')('bull'); var uuid = require('uuid'); +var commands = require('./commands/'); + /** Gets or creates a new Queue with the given name. @@ -138,7 +140,7 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ // // Keep track of cluster clients for redlock - // + // (Redlock is not used ATM.) this.clients = [this.client]; if (redisOptions.clients) { this.clients.push.apply(this.clients, redisOptions.clients); @@ -183,7 +185,6 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ }); }); - var events = [ 'delayed', 'paused', @@ -193,7 +194,9 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ this._initializing = Promise.all(initializers).then(function(){ return Promise.all(events.map(function(event){ return _this.eclient.subscribe(_this.toKey(event)) - })) + })).then(function(){ + return commands(_this.client); + }); }).then(function(){ debuglog(name + ' queue ready'); _this.emit('ready'); @@ -251,10 +254,12 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ // Init delay timestamp. // this.delayedTimestamp = Number.MAX_VALUE; - scripts.updateDelaySet(this, Date.now()).then(function(timestamp){ - if(timestamp){ - _this.updateDelayTimer(timestamp); - } + this.isReady().then(function(){ + scripts.updateDelaySet(_this, Date.now()).then(function(timestamp){ + if(timestamp){ + _this.updateDelayTimer(timestamp); + } + }); }); // @@ -384,7 +389,10 @@ Queue.prototype.process = function(name, concurrency, handler){ this.setHandler(name, handler); - return this.start(concurrency); + var _this = this; + return this.isReady().then(function(){ + return _this.start(concurrency); + }); }; Queue.prototype.start = function(concurrency){ @@ -424,10 +432,7 @@ interface JobOptions @param opts: JobOptions Options for this job. */ Queue.prototype.add = function(name, data, opts){ - var _this = this; - return this.isReady().then(function(){ - return Job.create(_this, name, data, opts); - }); + return Job.create(this, name, data, opts); }; /** @@ -609,9 +614,6 @@ Queue.prototype.updateDelayTimer = function(newDelayedTimestamp){ Queue.prototype.moveUnlockedJobsToWait = function(){ var _this = this; - // - // This should not be needed! - // if(this.closed){ return Promise.resolve(); } @@ -643,8 +645,6 @@ Queue.prototype.startMoveUnlockedJobsToWait = function() { } }; -// Idea to be able to know when a processing promise is finished, just store the promise - Queue.prototype.processJobs = function(index, resolve, reject){ var _this = this; var processJobs = this.processJobs.bind(this, index, resolve, reject); @@ -674,7 +674,6 @@ Queue.prototype.processJob = function(job){ } // - // TODO: // There are two cases to take into consideration regarding locks. // 1) The lock renewer fails to renew a lock, this should make this job // unable to complete, since some other worker is also working on it. @@ -719,7 +718,6 @@ Queue.prototype.processJob = function(job){ var error = err.cause || err; //Handle explicit rejection // See https://github.com/OptimalBits/bull/pull/415#issuecomment-269744735 - // job.takeLock(true /* renew */, false /* ensureActive */).then( function(/*lock*/) { return job.moveToFailed(err).then(function(){ return _this.distEmit('failed', job, error, 'active'); }).finally(function(){ diff --git a/lib/scripts.js b/lib/scripts.js index c3f40eb6a..7a023a160 100644 --- a/lib/scripts.js +++ b/lib/scripts.js @@ -10,54 +10,29 @@ var Promise = require('bluebird'); var _ = require('lodash'); var debuglog = require('debuglog')('bull'); -var Redlock = require('bull-redlock'); +// TO Deprecate. function execScript(client, hash, lua, numberOfKeys){ - var args = _.drop(arguments, 4); + var args = _.drop(arguments, 4); - if(!client[hash]){ - debuglog(hash, lua, args); - client.defineCommand(hash, { numberOfKeys: numberOfKeys, lua: lua }); - } - - return client[hash](args); -} - -function runScript(client, hash, lua, keys, args){ if(!client[hash]){ - debuglog(hash, lua, keys, args); - client.defineCommand(hash, { numberOfKeys: keys.length, lua: lua }); + debuglog(hash, lua, args); + client.defineCommand(hash, { numberOfKeys: numberOfKeys, lua: lua }); } - var params = keys.concat(args); - return client[hash](params); -} - -function isCommandDefined(client, hash){ - return !!client[hash]; -} - -var path = require('path'); -var fs = require('fs'); -function loadScriptIfNeeded(client, name){ - if(!client[name]){ - return fs.readFileSync(path.join(__dirname, 'scripts/' + name + '.lua')).toString(); - } + return client[hash](args); } var scripts = { isJobInList: function(client, listKey, jobId){ - var lua = loadScriptIfNeeded(client, 'isJobInList'); - - return runScript(client, 'isJobInList', lua, [listKey], [jobId]).then(function(result){ + return client.isJobInList([listKey, jobId]).then(function(result){ return result === 1; }); }, addJob: function(client, toKey, job, opts){ var queue = job.queue; - var lua = loadScriptIfNeeded(client, 'addJob'); - + var keys = _.map(['wait', 'paused', 'meta-paused', 'added', 'id', 'delayed', 'priority'], function(name){ return toKey(name); }); @@ -75,12 +50,10 @@ var scripts = { opts.lifo ? "LIFO" : "FIFO" ]; - return runScript(client, 'addJob', lua, keys, args); + return client.addJob(keys.concat(args)); }, pause: function(queue, pause) { - var lua = loadScriptIfNeeded(client, 'pause'); - var src, dst; if(pause){ src = 'wait'; @@ -94,12 +67,10 @@ var scripts = { return queue.toKey(name); }); - return runScript(queue.client, 'pause', lua, keys, [pause ? 'paused' : 'resumed']) + return queue.client.pause(keys.concat([pause ? 'paused' : 'resumed'])); }, moveToActive: function(queue){ - var lua = loadScriptIfNeeded(queue.client, 'moveToActive'); - var keys = _.map([ 'wait', 'active', @@ -114,7 +85,7 @@ var scripts = { queue.LOCK_DURATION ]; - return runScript(queue.client, 'moveToActive', lua, keys, args).then(function(result){ + return queue.client.moveToActive(keys.concat(args)).then(function(result){ if(result){ var jobData = result[0]; if(jobData.length){ @@ -126,9 +97,8 @@ var scripts = { }); }, - moveToFinished: function(job, val, propVal, shouldRemove, target, ignoreLock){ + moveToFinishedArgs: function(job, val, propVal, shouldRemove, target, ignoreLock){ var queue = job.queue; - var lua = loadScriptIfNeeded(queue.client, 'moveToFinished'); var keys = _.map([ 'active', @@ -147,29 +117,36 @@ var scripts = { shouldRemove ? "1" : "0" ]; - return runScript(queue.client, 'moveToFinished', lua, keys, args); + return keys.concat(args); + }, + + moveToFinished: function(job, val, propVal, shouldRemove, target, ignoreLock){ + var args = scripts.moveToFinishedArgs(job, val, propVal, shouldRemove, target, ignoreLock); + return job.queue.client.moveToFinished(args); }, moveToCompleted: function(job, returnvalue, removeOnComplete, ignoreLock){ return scripts.moveToFinished(job, returnvalue, 'returnvalue', removeOnComplete, 'completed', ignoreLock); }, + moveToFailedArgs: function(job, failedReason, removeOnFailed, ignoreLock){ + return scripts.moveToFinishedArgs(job, failedReason, 'failedReason', removeOnFailed, 'failed', ignoreLock); + }, + moveToFailed: function(job, failedReason, removeOnFailed, ignoreLock){ - return scripts.moveToFinished(job, failedReason, 'failedReason', removeOnFailed, 'failed', ignoreLock); + var args = scripts.moveToFailedArgs(job, failedReason, 'failedReason', removeOnFailed, 'failed', ignoreLock); + return scripts.moveToFinished(args); }, isFinished: function(job){ - var lua = loadScriptIfNeeded(job.queue.client, 'isFinished'); var keys = _.map(['completed', 'failed'], function(key){ return job.queue.toKey(key); }); - return runScript(job.queue.client, 'isFinished', lua, keys, [job.id]); + return job.queue.client.isFinished(keys.concat([job.id])); }, - moveToDelayed: function(queue, jobId, context){ - var lua = loadScriptIfNeeded(queue.client, 'moveToDelayed'); - + moveToDelayedArgs: function(queue, jobId, context){ // // Bake in the job id first 12 bits into the timestamp // to guarantee correct execution order of delayed jobs @@ -193,8 +170,12 @@ var scripts = { return queue.toKey(name); } ); + return keys.concat([JSON.stringify(context), jobId]); + }, - return runScript(queue.client, 'moveToDelayed', lua, keys, [JSON.stringify(context), jobId]).then(function(result){ + moveToDelayed: function(queue, jobId, context){ + var args = scripts.moveToDelayedArgs(queue, jobId, context); + return queue.client.moveToDelayed(args).then(function(result){ if(result === -1){ throw new Error('Missing Job ' + jobId + ' when trying to move from active to delayed'); } @@ -202,8 +183,6 @@ var scripts = { }, remove: function(queue, jobId){ - var lua = loadScriptIfNeeded(queue.client, 'removeJob'); - var keys = _.map([ 'active', 'wait', @@ -216,25 +195,19 @@ var scripts = { } ); - return runScript(queue.client, 'removeJob', lua, keys, [jobId, queue.token]); + return queue.client.removeJob(keys.concat([jobId, queue.token])); }, extendLock: function(queue, jobId){ - var lua = loadScriptIfNeeded(queue.client, 'extendLock'); - - return runScript(queue.client, 'extendLock', lua, [queue.toKey(jobId) + ':lock'], [queue.token, queue.LOCK_DURATION]); + return queue.client.extendLock([queue.toKey(jobId) + ':lock', queue.token, queue.LOCK_DURATION]); }, releaseLock: function(queue, jobId){ - var lua = loadScriptIfNeeded(queue.client, 'releaseLock'); - - return runScript(queue.client, 'releaseLock', lua, [queue.toKey(jobId) + ':lock'], [queue.token]); + return queue.client.releaseLock([queue.toKey(jobId) + ':lock', queue.token]); }, takeLock: function(queue, job){ - var lua = loadScriptIfNeeded(queue.client, 'takeLock'); - - return runScript(queue.client, 'takeLock', lua, [job.lockKey()], [queue.token, queue.LOCK_DURATION]); + return queue.client.takeLock([job.lockKey(), queue.token, queue.LOCK_DURATION]); }, /** @@ -242,8 +215,6 @@ var scripts = { top of the wait queue (so that it will be processed as soon as possible) */ updateDelaySet: function(queue, delayedTimestamp){ - var lua = loadScriptIfNeeded(queue.client, 'updateDelaySet'); - var keys = _.map([ 'delayed', 'active', @@ -254,7 +225,7 @@ var scripts = { var args = [queue.toKey(''), delayedTimestamp]; - return runScript(queue.client, 'updateDelaySet', lua, keys, args); + return queue.client.updateDelaySet(keys.concat(args)); }, /** @@ -266,13 +237,11 @@ var scripts = { * (e.g. if the job handler keeps crashing), we limit the number stalled job recoveries to MAX_STALLED_JOB_COUNT. */ moveUnlockedJobsToWait: function(queue){ - var lua = loadScriptIfNeeded(queue.client, 'moveUnlockedJobsToWait'); - var keys = _.map(['active', 'wait', 'failed', 'added'], function(key){ return queue.toKey(key); }); var args = [queue.MAX_STALLED_JOB_COUNT, queue.toKey(''), Date.now()]; - return runScript(queue.client, 'moveUnlockedJobsToWait', lua, keys, args); + return queue.client.moveUnlockedJobsToWait(keys.concat(args)); }, // TODO: Refactor into lua script @@ -346,19 +315,22 @@ var scripts = { return execScript.apply(scripts, args); }, - retryJob: function(job){ + retryJobArgs: function(job){ var queue = job.queue; var jobId = job.id; - var lua = loadScriptIfNeeded(queue.client, 'retryJob'); - var keys = _.map(['active', 'wait', jobId, 'added'], function(name){ return queue.toKey(name); }); var pushCmd = (job.opts.lifo ? 'R' : 'L') + 'PUSH'; - return runScript(queue.client, 'retryJob', lua, keys, [pushCmd, jobId]).then(function(result){ + return keys.concat([pushCmd, jobId]); + }, + + retryJob: function(job){ + var args = scripts.retryJobArgs(job); + return queue.client.retryJob(args).then(function(result){ if(result === -1){ throw new Error('Missing Job ' + jobId + ' during retry'); } @@ -380,7 +352,6 @@ var scripts = { * -2 means the job was not found in the expected set */ reprocessJob: function(job, options) { - var lua = loadScriptIfNeeded(job.queue.client, 'reprocessJob'); var queue = job.queue; var keys = [ @@ -396,7 +367,7 @@ var scripts = { (job.opts.lifo ? 'R' : 'L') + 'PUSH' ]; - return runScript(queue.client, 'reprocessJob', lua, keys, args); + return queue.client.reprocessJob(keys.concat(args)); } }; diff --git a/lib/scripts/index.js b/lib/scripts/index.js deleted file mode 100644 index f6c86a685..000000000 --- a/lib/scripts/index.js +++ /dev/null @@ -1,36 +0,0 @@ -/** - * Load redis lua scripts. - * The name of the script must have the following format: - * - * cmdName-numKeys.lua - * - * cmdName must be in camel case format. - * - * For example: - * moveToFinish-3.lua - * - */ - -var fs = require('fs'); -var path = require('path'); - -// -// for some very strange reason, defining scripts with this code results in this error -// when executing the scripts: ERR value is not an integer or out of range -// -module.exports = function(client){ - loadScripts(client, __dirname); -} - -function loadScripts(client, dir) { - fs.readdirSync(dir).filter(function (file) { - return path.extname(file) === '.lua'; - }).forEach(function (file) { - var longName = path.basename(file, '.lua'); - var name = longName.split('-')[0]; - var numberOfKeys = longName.split('-')[1]; - - var lua = fs.readFileSync(path.join(dir, file)).toString(); - client.defineCommand(name, { numberOfKeys: numberOfKeys, lua: lua }); - }); -}; From b8a942870d46a6229dbffa864c3c5618ce3ff051 Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sun, 16 Apr 2017 18:29:12 +0200 Subject: [PATCH 16/17] atomized job##moveToFailed --- lib/job.js | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/lib/job.js b/lib/job.js index f6192c237..7db3901bd 100644 --- a/lib/job.js +++ b/lib/job.js @@ -54,12 +54,13 @@ function addJob(queue, job){ Job.create = function(queue, name, data, opts){ var job = new Job(queue, name, data, opts); - - return addJob(queue, job).then(function(jobId){ - job.id = jobId; - queue.distEmit('waiting', job, null); - debuglog('Job added', jobId); - return job; + return queue.isReady().then(function(){ + return addJob(queue, job).then(function(jobId){ + job.id = jobId; + queue.distEmit('waiting', job, null); + debuglog('Job added', jobId); + return job; + }); }); }; @@ -160,28 +161,38 @@ Job.prototype.discard = function(){ this._discarded = true; } -// -// This could be atomized using "multi", since performance is not so critical in the failed -// case. For this to work we need to be able to define the lua commands at start up time. -// Job.prototype.moveToFailed = function(err, ignoreLock){ var _this = this; - return this._saveAttempt(err).then(function() { + return new Promise(function(resolve, reject){ + var multi = _this.queue.client.multi(); + _this._saveAttempt(multi, err); + // Check if an automatic retry should be performed if(_this.attemptsMade < _this.opts.attempts && !_this._discarded){ // Check if backoff is needed var backoff = _this._getBackOff(); if(backoff){ // If so, move to delayed (need to unlock job in this case!) - return _this.moveToDelayed(Date.now() + backoff); + var args = scripts.moveToDelayedArgs(_this.queue, _this.id, Date.now() + backoff); + multi.moveToDelayed(args); }else{ // If not, retry immediately - return scripts.retryJob(_this); + multi.retryJob(scripts.retryJobArgs(_this), function(err, result){ + if(err){ + reject(); + }else{ + if(result === -1){ + reject(new Error('Missing Job ' + jobId + ' during retry')); + } + } + }); } } else { // If not, move to failed - return scripts.moveToFailed(_this, err.message, _this.opts.removeOnFail, ignoreLock); + var args = scripts.moveToFailedArgs(_this, err.message, _this.opts.removeOnFail, ignoreLock) + multi.moveToFinished(args); } + return multi.exec().then(resolve, reject); }); }; @@ -394,7 +405,7 @@ Job.prototype._getBackOff = function() { return backoff; }; -Job.prototype._saveAttempt = function(err){ +Job.prototype._saveAttempt = function(multi, err){ this.attemptsMade++; var params = { @@ -405,7 +416,7 @@ Job.prototype._saveAttempt = function(err){ params.stacktrace = JSON.stringify(this.stacktrace); params.failedReason = err.message; - return this.queue.client.hmset(this.queue.toKey(this.id), params); + multi.hmset(this.queue.toKey(this.id), params); }; Job.fromData = function(queue, raw, jobId){ From 34cc0c1383c735fec77b2a282c98b202c28cfd4c Mon Sep 17 00:00:00 2001 From: Manuel Astudillo Date: Sun, 16 Apr 2017 18:37:00 +0200 Subject: [PATCH 17/17] added slow poll for waiting jobs --- lib/queue.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/queue.js b/lib/queue.js index 323f12e7a..e53aaac06 100644 --- a/lib/queue.js +++ b/lib/queue.js @@ -276,6 +276,10 @@ var Queue = function Queue(name, redisPort, redisHost, redisOptions){ _this.emit('error', err); }); } + // + // Trigger a getNextJob (if worker is idling) + // + _this.emit('added'); }, POLLING_INTERVAL); // Bind these methods to avoid constant rebinding and/or creating closures