OptimalBits / bull

Premium Queue package for handling distributed jobs and messages in NodeJS.
Other
15.49k stars 1.43k forks source link

BUG: Inconsistent delay property #1833

Open tsebas opened 4 years ago

tsebas commented 4 years ago

Description

When a job is created with queue.add with the delay param in opts, the resulting job has the delay property with the assigned value. However, in the queue processor, the job that comes in has the corret delay value in the opts object, whilst the delay property has 0.

Minimal, Working Test code to reproduce the issue.

const bull = require('bull');

const q = new bull('queue', {
    redis: {host: 'localhost', port: 6380, password: null},
});

q.process(job => {
    console.log('process', job.delay, job.opts.delay);
});

q.add({}, {delay: 1}).then(job => console.log('add', job.delay, job.opts.delay));

Bull version

"bull": "^3.14.0"

Additional information

The above code generates the following output. I added a console.log on Job.fromJSON function in bull/lib/job.js:576

add 1 1
json { name: '__default__',
  data: '{}',
  opts: '{"delay":1,"attempts":1,"timestamp":1598439486198}',
  timestamp: '1598439486198',
  delay: '0',
  priority: '0',
  processedOn: '1598439486213' }
process 0 1

I didn't fully debug the code to understand where the most appropriate fix would go, but I suspect the only actually problematic line is bull/lib/job.js:588 job.delay = parseInt(json.delay);. Before this line, the job.delay is correctly built form the constructor.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sibelius commented 2 years ago

is this a bug?

tsebas commented 2 years ago

yes. as far as I can tell, it is inconsistent between job.delay and job.opts.delay

adrien-may commented 2 years ago

I think this is more or less intended. It seems the delay appears correctly as long as the delay is "running". Once the job is running or complete, delay is set to 0 (because it's no more delayed ?)

manast commented 2 years ago

Is this behavior creating any practical issue?

adrien-may commented 2 years ago

It is for sure misleading. Not sure what is the point to set it back to 0 after completion. As we already have access to its state and the timestamp of completion, I see no value here. I don't thing this leads to any bugs though.

tsebas commented 2 years ago

Well, in practice, as soon as I start relying on it for something and it suddenly is 0 when there is no obvious reason for it, then I'd say yes, it causes practical issues. However, I don't think it is necessarily a problem. Just that it is as @adrien-may says "misleading".

It is confusing that I compare 2 things that at face value should be the same, but then turn out not to be, or that one of them is state dependant. It is not obvious that there should be a difference.

Not entirely sure how this could be easily addressed, by changing code of fixing readme (as I'm not sure how intended this was original or what to write on the readme). But also wouldn't ask for much or for anything at all to be changed. I've voiced my concern that it is inconsistent. if this thread is documentation enough for people that face the same in the future, that is enough I guess.

manast commented 2 years ago

I understand, but delay is meant to be a private property so you should not make too many assumptions about its behavior as it would be implementation dependant.

tsebas commented 2 years ago

that is fair! I'm just reporting what looks like inconsistent behaviour. It is never made clear that job.delay behaves potentially differently from the opts and that is what my ticket was intended to point out. If this is all intended behaviour, I'm ok with closing this as is! Let this convo serve as documentation if anyone comes looking. :)