Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Inconsistent delay property #1833

Open
tsebas opened this issue Aug 26, 2020 · 9 comments
Open

BUG: Inconsistent delay property #1833

tsebas opened this issue Aug 26, 2020 · 9 comments
Labels

Comments

@tsebas
Copy link

tsebas commented Aug 26, 2020

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
Copy link

stale bot commented Jul 12, 2021

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.

@stale stale bot added the wontfix label Jul 12, 2021
@manast manast added bug and removed wontfix labels Jul 13, 2021
@sibelius
Copy link

is this a bug?

@tsebas
Copy link
Author

tsebas commented Mar 22, 2022

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

@adrien-may
Copy link

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
Copy link
Member

manast commented May 12, 2022

Is this behavior creating any practical issue?

@adrien-may
Copy link

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
Copy link
Author

tsebas commented May 12, 2022

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
Copy link
Member

manast commented May 12, 2022

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
Copy link
Author

tsebas commented May 12, 2022

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants