@user_script:86: user_script:86: too many results to unpack

[hasparus]

Hi! That's my first issue, so firstly I wanted to thank you folks for BullMQ!
We're using it quite a lot with @andreasasprou and this is the first problem I encountered.

After scaling up our workers, we've noticed a new error

ERR Error running script (call to f_890be59efdd64a4df5f5121df9680b166a884036): @user_script:86: user_script:86: too many results to unpack

I looked around the repository, and found unpack in two places

Once in updateDelaySet-7.lua with a table at most 1000-elements big, and the second time in moveStalledJobsToWait-8.lua in line 86 with all jobs from KEYS[3] 'active'.

bullmq/src/commands/moveStalledJobsToWait-8.lua

Lines 85 to 86 in 297ec30

	local active = rcall('LRANGE', KEYS[3], 0, -1)
	if (#active > 0) then rcall('SADD', KEYS[1], unpack(active)) end

I'm reading that calling unpack with more than 8k elements is known to crash.

• https://devforum.roblox.com/t/unpack-limit-fix/213862
• https://stackoverflow.com/questions/19202367/how-to-avoid-redis-calls-in-lua-script-limitations

I wanted to ask about limits of bullmq — should we try to scale Bull and Redis instances when dealing tens of thousands of jobs?
Or is this just a software bug, that could be solved with iteration over active and calling unpack with batches of 7000 (unpack(active, from_index, to_index))?

[hasparus]

Here is a diff with a potential solution.

diff --git a/node_modules/bullmq/dist/commands/moveStalledJobsToWait-8.lua b/node_modules/bullmq/dist/commands/moveStalledJobsToWait-8.lua
index 0285201..cce3af5 100644
--- a/node_modules/bullmq/dist/commands/moveStalledJobsToWait-8.lua
+++ b/node_modules/bullmq/dist/commands/moveStalledJobsToWait-8.lua
@@ -81,9 +81,27 @@ if (#stalling > 0) then
     end
 end
 
+-- Divide n into equal size batches
+local function batches(n, batchSize)
+  local i = 0
+
+  return function()
+    local from = i * batchSize + 1
+    i = i + 1
+    if (from <= n) then
+      local to = math.min(from + batchSize - 1, n)
+      return from, to
+    end
+  end
+end
+
 -- Mark potentially stalled jobs
 local active = rcall('LRANGE', KEYS[3], 0, -1)
-if (#active > 0) then rcall('SADD', KEYS[1], unpack(active)) end
+if (#active > 0) then
+  for from, to in batches(#jobs, 7000) do
+    rcall('SADD', KEYS[1], unpack(jobs, from, to))
+  end
+end
 
 local maxEvents = rcall("HGET", KEYS[6], "opts.maxLenEvents")
 if maxEvents then rcall("XTRIM", KEYS[8], "MAXLEN", "~", maxEvents) end
diff --git a/node_modules/bullmq/src/commands/moveStalledJobsToWait-8.lua b/node_modules/bullmq/src/commands/moveStalledJobsToWait-8.lua
index 0285201..cce3af5 100644
--- a/node_modules/bullmq/src/commands/moveStalledJobsToWait-8.lua
+++ b/node_modules/bullmq/src/commands/moveStalledJobsToWait-8.lua
@@ -81,9 +81,27 @@ if (#stalling > 0) then
     end
 end
 
+-- Divide n into equal size batches
+local function batches(n, batchSize)
+  local i = 0
+
+  return function()
+    local from = i * batchSize + 1
+    i = i + 1
+    if (from <= n) then
+      local to = math.min(from + batchSize - 1, n)
+      return from, to
+    end
+  end
+end
+
 -- Mark potentially stalled jobs
 local active = rcall('LRANGE', KEYS[3], 0, -1)
-if (#active > 0) then rcall('SADD', KEYS[1], unpack(active)) end
+if (#active > 0) then
+  for from, to in batches(#jobs, 7000) do
+    rcall('SADD', KEYS[1], unpack(jobs, from, to))
+  end
+end
 
 local maxEvents = rcall("HGET", KEYS[6], "opts.maxLenEvents")
 if maxEvents then rcall("XTRIM", KEYS[8], "MAXLEN", "~", maxEvents) end

[manast]

Hello @hasparus. It seems that you indeed have reached a limit in the lua script implementation used by redis.
thanks for the patch, I am studying it, but my first impression is that it could be a bit slow, since iterating arrays in .lua is not super fast. I will check if there is some native lua way to divide arrays in batches.

[manast]

ok, so I think it would be more optimal to do use redis LRANGE to get the jobids in batches. So first doing a LLEN will return the total length, if it is larger than 8k start getting in batches with LRANGE until the whole list has been consumed.
I can create a PR for this but I cannot give an ETA. If you have time to do it quickly I would accept the PR if it passes all the tests (would be greate to even have a test for the case where there are more than 8k active jobs at once).

[hasparus]

but my first impression is that it could be a bit slow, since iterating arrays in .lua is not super fast. I will check if there is some native lua way to divide arrays in batches.

But, I'm not iterating the array?... at least I don't think so, I last wrote Lua a few years ago.
My patch in "pseudocode" should be more or less

window = { from: 0, to: 7000 }
until window.to > active.length
  SADD(KEYS[1], ...take(jobs, from: window.from, to: window.to)
  move window by 7000 to the right

I assume unpack does some iteration here, but iterating through entire active and iterating through active in batches of 7000 shouldn't be much different (right? I'm truly not a Lua guy). My code does more SADD calls, though, and I admit, I may have overengineered it a bit with that generator.

If you have time to do it quickly I would accept the PR if it passes all the tests (would be greate to even have a test for the case where there are more than 8k active jobs at once).

I fiddled for a moment with tests and I didn't manage to fail them with tens of thousands jobs 😓
The bug can be reproduced locally on my machine and it happened on our production, so I may have approached it wrongly.

[manast]

But, I'm not iterating the array?... at least I don't think so, I last wrote Lua a few years ago.

ah ok, sorry I did not understand the code completely at a fast glance. So basically it is just unpack consuming different parts of the array which we could presume is fast enough.

I fiddled for a moment with tests and I didn't manage to fail them with tens of thousands jobs 😓
The bug can be reproduced locally on my machine and it happened on our production, so I may have approached it wrongly.

Could it be the version of redis? Maybe the one you use in production has this limitation but the one running locally doesn't? Btw, the amount of jobs wouldn't be the culprit here if I understand the issue correctly, but rather the amount of jobs plus the amount of concurrent workers (so that you can end having more than 8k active jobs at a given time.

[hasparus]

Maybe the one you use in production has this limitation but the one running locally doesn't?

Well, I can crash it with my app even on locally running Redis, but I failed to crash tests here.
I am familiar with this Lua limitation, and it's quite well known in the internet, so question here is when can active become larger than 8000 items?

[manast]

@hasparus you need to have a number of workers times concurrency per worker > 8000 and a queue that has enough items to keep all the workers busy at max capacity.
Anyway, I think we should merge this fix even if we cannot reproduce it in a test case, because that case would be really heavy to run anyway in a CI system.

[orgalaf]

getting this too!
version 1.15.1

runner-77488cd598-mvpz9 runner-0 unhandledRejection ReplyError: ERR Error running script (call to f_890be59efdd64a4df5f5121df9680b166a884036): @user_script:86: user_script:86: too many results to unpack
runner-77488cd598-mvpz9 runner-0     at parseError (/usr/local/lib/node_module/.../runner/node_modules/redis-parser/lib/parser.js:179:12)
runner-77488cd598-mvpz9 runner-0     at parseType (/usr/local/lib/node_modules/.../runner/node_modules/redis-parser/lib/parser.js:302:14) {
runner-77488cd598-mvpz9 c-0   command: {
runner-77488cd598-mvpz9 c-0     name: 'evalsha',
runner-77488cd598-mvpz9 c-0     args: [
runner-77488cd598-mvpz9 c-0       '890be59efdd64a4df5f5121df9680b166a884036',
runner-77488cd598-mvpz9 c-0       '8',
runner-77488cd598-mvpz9 c-0       'bull:some-queue:default:stalled',
runner-77488cd598-mvpz9 c-0       'bull:some-queue:default:wait',
runner-77488cd598-mvpz9 c-0       'bull:some-queue:default:active',
runner-77488cd598-mvpz9 c-0       'bull:some-queue:default:failed',
runner-77488cd598-mvpz9 runner-0       'bull:some-queue:default:stalled-check',
runner-77488cd598-mvpz9 runner-0       'bull:some-queue:default:meta',
runner-77488cd598-mvpz9 runner-0       'bull:some-queue:default:paused',
runner-77488cd598-mvpz9 runner-0       'bull:some-queue:default:events',
runner-77488cd598-mvpz9 runner-0       '1',
runner-77488cd598-mvpz9 runner-0       'bull:some-queue:default:',
runner-77488cd598-mvpz9 runner-0       '1623472439517',
runner-77488cd598-mvpz9 runner-0       '30000'
runner-77488cd598-mvpz9 runner-0     ]
runner-77488cd598-mvpz9 runner-0   }
runner-77488cd598-mvpz9 runner-0 }

[hasparus]

Anyway, I think we should merge this fix even if we cannot reproduce it in a test case, because that case would be really heavy to run anyway in a CI system.

Hey @manast, since it's also happening for @orgalaf, should I create a PR with the change I mentioned in #422 (comment)? We worked around the problem, so it doesn't affect me anymore, but it could save a surprise for other people.

[manast]

@hasparus sure, that would be great if it saves time and issues to other users.