@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Fix an issue where PhabricatorWorkerLeaseQuery may lease different tasks than it selects
Summary:
We lock tasks by setting `leaseOwner` to a unique value, but the value is currently unique-to-the-process rather than unique-to-the-query. This means that if a process leases a task, then leases another task, both tasks will have the same `leaseOwner`. This can cause an issue where we go to select the task we just leased and get the other task instead, if we aren't careful about the select construction.
We can avoid this by being clever and making sure the select is constructed correctly, but making the `leaseOwner` unique to the query is much simpler and more foolproof. This guarantees we always select only the rows we just leased.
Also remove `PhabricatorGoodForNothingWorker` since `PhabricatorTestWorker` fills its role of allowing things to be tested, and simplify the unit tests since we don't need to be clever about avoiding this issue any more.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3862
+16
-45
-2
src/__phutil_library_map__.php
-2
src/__phutil_library_map__.php
···
775
775
'PhabricatorGarbageCollectorDaemon' => 'infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php',
776
776
'PhabricatorGitGraphStream' => 'applications/repository/daemon/PhabricatorGitGraphStream.php',
777
777
'PhabricatorGlobalLock' => 'infrastructure/util/PhabricatorGlobalLock.php',
778
-
'PhabricatorGoodForNothingWorker' => 'infrastructure/daemon/workers/worker/PhabricatorGoodForNothingWorker.php',
779
778
'PhabricatorHandleObjectSelectorDataView' => 'applications/phid/handle/view/PhabricatorHandleObjectSelectorDataView.php',
780
779
'PhabricatorHash' => 'infrastructure/util/PhabricatorHash.php',
781
780
'PhabricatorHeaderView' => 'view/layout/PhabricatorHeaderView.php',
···
1982
1981
'PhabricatorFormExample' => 'PhabricatorUIExample',
1983
1982
'PhabricatorGarbageCollectorDaemon' => 'PhabricatorDaemon',
1984
1983
'PhabricatorGlobalLock' => 'PhutilLock',
1985
-
'PhabricatorGoodForNothingWorker' => 'PhabricatorWorker',
1986
1984
'PhabricatorHeaderView' => 'AphrontView',
1987
1985
'PhabricatorHelpController' => 'PhabricatorController',
1988
1986
'PhabricatorHelpKeyboardShortcutController' => 'PhabricatorHelpController',
+2
-2
src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
+2
-2
src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
···
47
47
$task1 = $this->scheduleTask();
48
48
$task2 = $this->scheduleTask();
49
49
50
-
$this->expectNextLease($task1)->executeTask();
50
+
$this->expectNextLease($task1);
51
51
$this->expectNextLease($task2);
52
52
}
53
53
···
61
61
$task1->setLeaseExpires(time() - 100000);
62
62
$task1->forceSaveWithoutLease();
63
63
64
-
$this->expectNextLease($task2)->executeTask();
64
+
$this->expectNextLease($task2);
65
65
$this->expectNextLease($task1);
66
66
}
67
67
+14
-15
src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
+14
-15
src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
···
25
25
26
26
const PHASE_UNLEASED = 'unleased';
27
27
const PHASE_EXPIRED = 'expired';
28
-
const PHASE_SELECT = 'select';
29
28
30
29
const DEFAULT_LEASE_DURATION = 60; // Seconds
31
30
···
92
91
'SELECT task.*, taskdata.data _taskData, UNIX_TIMESTAMP() _serverTime
93
92
FROM %T task LEFT JOIN %T taskdata
94
93
ON taskdata.id = task.dataID
95
-
%Q %Q %Q',
94
+
WHERE leaseOwner = %s AND leaseExpires > UNIX_TIMESTAMP()
95
+
%Q %Q',
96
96
$task_table->getTableName(),
97
97
$taskdata_table->getTableName(),
98
-
$this->buildWhereClause($conn_w, self::PHASE_SELECT),
98
+
$lease_ownership_name,
99
99
$this->buildOrderClause($conn_w),
100
100
$this->buildLimitClause($conn_w, $limit));
101
101
···
116
116
}
117
117
118
118
private function buildWhereClause(AphrontDatabaseConnection $conn_w, $phase) {
119
+
119
120
$where = array();
120
121
121
122
switch ($phase) {
···
125
126
case self::PHASE_EXPIRED:
126
127
$where[] = 'leaseExpires < UNIX_TIMESTAMP()';
127
128
break;
128
-
case self::PHASE_SELECT:
129
-
$where[] = qsprintf(
130
-
$conn_w,
131
-
'leaseOwner = %s',
132
-
$this->getLeaseOwnershipName());
133
-
$where[] = 'leaseExpires > UNIX_TIMESTAMP()';
134
-
break;
135
129
default:
136
130
throw new Exception("Unknown phase '{$phase}'!");
137
131
}
···
155
149
}
156
150
157
151
private function getLeaseOwnershipName() {
158
-
static $name = null;
159
-
if ($name === null) {
160
-
$name = getmypid().':'.time().':'.php_uname('n');
161
-
}
162
-
return $name;
152
+
static $sequence = 0;
153
+
154
+
$parts = array(
155
+
getmypid(),
156
+
time(),
157
+
php_uname('n'),
158
+
++$sequence,
159
+
);
160
+
161
+
return implode(':', $parts);
163
162
}
164
163
165
164
}
-26
src/infrastructure/daemon/workers/worker/PhabricatorGoodForNothingWorker.php
-26
src/infrastructure/daemon/workers/worker/PhabricatorGoodForNothingWorker.php
···
1
-
<?php
2
-
3
-
/*
4
-
* Copyright 2012 Facebook, Inc.
5
-
*
6
-
* Licensed under the Apache License, Version 2.0 (the "License");
7
-
* you may not use this file except in compliance with the License.
8
-
* You may obtain a copy of the License at
9
-
*
10
-
* http://www.apache.org/licenses/LICENSE-2.0
11
-
*
12
-
* Unless required by applicable law or agreed to in writing, software
13
-
* distributed under the License is distributed on an "AS IS" BASIS,
14
-
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15
-
* See the License for the specific language governing permissions and
16
-
* limitations under the License.
17
-
*/
18
-
19
-
/**
20
-
* Trivial example worker; processes tasks which require no work very slowly.
21
-
*/
22
-
final class PhabricatorGoodForNothingWorker extends PhabricatorWorker {
23
-
protected function doWork() {
24
-
sleep(10);
25
-
}
26
-
}