edgurgel/verk

Poison 3.0 causes job decoding exceptions

keyan opened this issue · 5 comments

keyan commented

I upgraded to 1.1.1 a few weeks ago and after a few days of smooth sailing my application was crashing with the following error:

terminating
** (CaseClauseError) no case clause matching: {:error, :invalid, 0}
    (verk) lib/verk/job.ex:22: Verk.Job.decode/1
    (verk) lib/verk/workers_manager.ex:150: anonymous fn/2 in Verk.WorkersManager.handle_info/2

Turns out I had inadvertently bumped Poison to 3.0.0 at the same time. 3.0.0 introduces a change to the return tuple that breaks the handling added in #91. The method signature change is documented in: devinus/poison#149. It seems that this signature might be changed back in Poison 4.0.0.

Options:

  1. add handling here for the new return signature
  2. choose to not support Poison 3.0.0 and plan that 4.0.0 will revert the change
edgurgel commented

Nice finding! I wonder how the JSON was invalid in the first place 🤔 Was this job added by Verk or another Sidekiq-compatible client?

We definitely need to fix this behaviour

keyan commented

Not even as fancy as another client, while we are migrating stacks many of our jobs are enqueued in php via the following code:

$task_body = [
  'class' => 'Exworker.PhpWorker',
  'args' => [$worker, $data],
  'queue' => $queue_name,
  'jid' => uniqid(),
  'max_retry_count' => $max_retry_count
];

$this->_redis->rPush('queue:' . $queue_name, json_encode($task_body));

So I'm pretty sure the issue is on our end, maybe relating to the standard library PHP json encoder. There isn't much code there that could be broken, but we aren't doing validation on the data so it is possible there is some bad usage in the codebase somewhere?

Regardless, it isn't an issue with Verk.enqueue, but I do think it is worth keeping the error handling code in case other users are enqueuing using something aside from Verk.

edgurgel commented

We could patch Verk right now by supporting this new {:error clause and be fine with either Poison 3.0 or 4.0. What do you think?

keyan commented

Sure, seems fine, the fix is simple: #145

edgurgel commented

I will release a new version later today! Thank you again!