-
Notifications
You must be signed in to change notification settings - Fork 419
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
fix(worker): allow retrieving concurrency value #2883
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/classes/worker.ts
Outdated
@@ -395,6 +395,10 @@ export class Worker< | |||
this.opts.concurrency = concurrency; | |||
} | |||
|
|||
get concurrency() { | |||
return this.opts.concurrency; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bug where this.opts.concurrency should not be set, instead it should be this.concurrency = concurrency at line 395. opts should be immutable as we threat other options like priority or delay. If you change that line, this getter may not be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is resulting in unending recursion: "RangeError: Maximum call stack size exceeded"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgozdz what @roggervalf means is that we should not replace this.opts.concurrency as this is mutating the opts object which was passed in the constructor. Instead you can define a private property like _concurrency, and use this instead of this.opts.concurrency. In the constructor you set _concurrency to a default value if this.opts.concurrency is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly speaking It would be far mote elegant to have a explicit setter like "setConcurrency" instead of an implicit setter, but this would imply a breaking change, so we cannot do that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgozdz you can try doing:
set concurrency(concurrency: number) {
if (
typeof concurrency !== 'number' ||
concurrency < 1 ||
!isFinite(concurrency)
) {
throw new Error('concurrency must be a finite number greater than 0');
}
this._concurrency = concurrency;
}
get concurrency(): number {
return this._concurrency;
}
on worker constructor you may need to replace https://github.com/taskforcesh/bullmq/blob/master/src/classes/worker.ts#L241 as:
this._concurrency = this.opts.concurrency;
and add a private attribute type after this one https://github.com/taskforcesh/bullmq/blob/master/src/classes/worker.ts#L182 like:
private _concurrency:number;
also in order to use this new attribute you should replace this.opts.concurrency to use this._concurrency at worker level
This issue fixes #2880 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, thank you
c59d59d
to
cb1fb5e
Compare
No description provided.