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

[BUGFIX #317] Make queueJob threadsafe #342

Open
wants to merge 1 commit into
base: 4
Choose a base branch
from

Conversation

danaenz
Copy link

@danaenz danaenz commented Mar 26, 2021

Adds a transaction lock to JobDescriptor lookup to prevent job creation with duplicate signatures.

Functionality improvements

  • Wrapped queueJob read/write functionality in withTransaction()
  • Added exception handling

Readability

  • Separated functionality into protected methods for readability

Fixes Issue 317

- Separate functionality into protected methods for readability
- Wrap queueJob read/write functionality in withTransaction()
return $this->findOrMakeJobDescriptorFromSignature($signature, $job, $jobDescriptor, $startAfter);
} catch (\Throwable $e) {
// note that error here may not be an issue as failing to acquire a job lock is a valid state
// which happens when other process claimed the job lock first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comments should be updated to describe the job signature, not the job lock as that is a different feature. Please also update the log message so this feature is not confused with the job locking.

@brynwhyman
Copy link

Hi @danaenz a quick follow up to see if you're still planning on addressing the feedback in this PR? :)

Copy link
Collaborator

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks good overall

* @param null|string $startAfter
* @return int|null
*/
protected function findOrMakeJobDescriptorFromSignature($signature, $job, $jobDescriptor, $startAfter)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be made private to reduce the API surface. There's an extension hook available if it needs to be extended.

* @param null $queueName
* @return QueuedJobDescriptor
*/
protected function createJobDescriptor(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should change this from protected to private

{
// Start a transaction which will hold until we have a lock on this signature.
return DB::get_conn()->withTransaction(function () use ($signature, $job, $jobDescriptor, $startAfter) {
$query = 'SELECT "ID" FROM "QueuedJobDescriptor" WHERE "Signature" = ? FOR UPDATE';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of an edge case, though should read the tableName of QueuedJobDescriptor first incase it's been changed (table names are configuraable

$tableName = Config::inst()->get(QueuedJobDescriptor::class, 'table_name', Config::UNINHERITED);


// still no user - fallback to current user
if ($userId === null) {
if (Security::getCurrentUser() && Security::getCurrentUser()->exists()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should put this to a variable so that it doesn't need to be called 3 separate times times
$member = Security::getCurrentUser()

QueuedJob $job,
$signature,
$startAfter = null,
$userId = null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$userId = null,
$userID = null,

Convention is to capitilise the d in ID

Also update the references in this function to $userID

$this->startJob($jobDescriptor, $startAfter);

return $jobDescriptor->ID;
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return false;
return 0;

PHPDOC function signature is to return an int

$userId = $job->getRunAsMemberID();
}
// Create the initial object
$jobDescriptor = $this->createJobDescriptor($job, $signature, $startAfter, $userId, $queueName);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of weird creating the job and then passing it to findOrMakeJobDescriptorFromSignature

Seems like we should call createJobDescriptor() inside findOrMakeJobDescriptorFromSignature on if we were unable to find a jobDescriptor from the signature

$this->copyJobToDescriptor($job, $jobDescriptor);

// Write the record
$jobDescriptorID = $jobDescriptor->write();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$jobDescriptorID variable is unused?


$this->startJob($jobDescriptor, $startAfter);
} else {
$jobDescriptorID = $ID;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$jobDescriptorID appears to be unused, so we can just omit this?

protected function findOrMakeJobDescriptorFromSignature($signature, $job, $jobDescriptor, $startAfter)
{
// Start a transaction which will hold until we have a lock on this signature.
return DB::get_conn()->withTransaction(function () use ($signature, $job, $jobDescriptor, $startAfter) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the return? Database::withTransaction() doesn't have a return value?

@mfendeksilverstripe
Copy link
Contributor

@danaenz Please review the feedback on this PR and let me know if you need help with it ;-) Would be keen on getting this over the line.

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

Successfully merging this pull request may close these issues.

Job signature system is not thread safe
4 participants