[BUGFIX #317] Make queueJob threadsafe #342
Conversation
- 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 |
There was a problem hiding this comment.
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.
|
Hi @danaenz a quick follow up to see if you're still planning on addressing the feedback in this PR? :) |
| * @param null|string $startAfter | ||
| * @return int|null | ||
| */ | ||
| protected function findOrMakeJobDescriptorFromSignature($signature, $job, $jobDescriptor, $startAfter) |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
| $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; |
There was a problem hiding this comment.
| 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); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
$jobDescriptorID variable is unused?
|
|
||
| $this->startJob($jobDescriptor, $startAfter); | ||
| } else { | ||
| $jobDescriptorID = $ID; |
There was a problem hiding this comment.
$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) { |
There was a problem hiding this comment.
Do we need the return? Database::withTransaction() doesn't have a return value?
|
@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. |
Adds a transaction lock to JobDescriptor lookup to prevent job creation with duplicate signatures.
Functionality improvements
Readability
Fixes Issue 317