JobLock Plugin - beforePerform behaviour

[jamagalhaes]

Hey,

I found an issue related with JobLock Plugin. By default, JobLock re-enqueues a job if its key was already set. If, for some reason, renqueue fails the error will be caught in here:

node-resque/src/core/worker.ts

Lines 295 to 314 in 708ea4a

	} catch (error) {
	this.error = error;
	if (!triedAfterPerform) {
	try {
	await RunPlugins(
	this,
	"afterPerform",
	job.class,
	this.queue,
	this.jobs[job.class],
	job.args
	);
	} catch (error) {
	if (error && !this.error) {
	this.error = error;
	}
	}
	}
	return this.completeJob(!this.error, startedAt);
	}

and afterPerform stage will be executed anyway. This will cause JobLock key to be deleted and it will completely break the plugin purpose because it immediately allows other jobs(that share the same key) to be executed concurrently.

Possible solutions:

1. refactor Joblock beforePerform() stage to handle any possible errors when re-enqueing
2. don't call RunPlugins('afterPerform') if job perform wasn't effectively called.

Cheers

[evantahler]

I wonder if this is related to the changes that were just added with #772. We want to call afterPerform now, even if there is an error, so it looks like option (1) is the way to go.

Are you able to make a PR with this fix?

Thanks!

[jamagalhaes]

@evantahler I was wondering if it makes sense to call afterPerform even if perform wasn't executed at all. In my perspective, afterPerfom should only be called if job perform was executed and I think that was the main purpose of #772 - making sure afterPerform is called even if job perform throws an error.
But I believe this case is a little bit different because this error happens at beforePerform stage and job perform wasn't executed yet.
Tell me your thoughts.

Cheers

[evantahler]

Sounds like to be maximally flexible, we could always run both steps, but provide metadata about the success of previous steps, so the afterPerform might get:

{
  before: "success",
  run: "success",
  after: "pending"
}

This would be a new piece of API, so probably a breaking change to add? So yes, I think I agree it would be nice to call afterPerform even if there was no run... but we need the additional metadata to explain what happened. Without that, than I think we should not call afterPerform