Skip to content

Commit 44f0664

Browse files
author
epriestley
committedMar 6, 2018
Add a "lock log" for debugging where locks are being held
Summary: Depends on D19173. Ref T13096. Adds an optional, disabled-by-default lock log to make it easier to figure out what is acquiring and holding locks. Test Plan: Ran `bin/lock log --enable`, `--disable`, `--name`, etc. Saw sensible-looking output with log enabled and daemons restarted. Saw no additional output with log disabled and daemons restarted. Maniphest Tasks: T13096 Differential Revision: https://secure.phabricator.com/D19174
1 parent fd367ad commit 44f0664

11 files changed

+389
-6
lines changed
 

‎bin/lock

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
../scripts/setup/manage_lock.php

‎resources/sql/autopatches/20180305.lock.01.locklog.sql

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
CREATE TABLE {$NAMESPACE}_daemon.daemon_locklog (
2+
id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
3+
lockName VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT},
4+
lockReleased INT UNSIGNED,
5+
lockParameters LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT},
6+
lockContext LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT},
7+
dateCreated INT UNSIGNED NOT NULL,
8+
dateModified INT UNSIGNED NOT NULL
9+
) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};

‎scripts/setup/manage_lock.php

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
#!/usr/bin/env php
2+
<?php
3+
4+
$root = dirname(dirname(dirname(__FILE__)));
5+
require_once $root.'/scripts/init/init-script.php';
6+
7+
$args = new PhutilArgumentParser($argv);
8+
$args->setTagline(pht('manage locks'));
9+
$args->setSynopsis(<<<EOSYNOPSIS
10+
**lock** __command__ [__options__]
11+
Manage locks.
12+
13+
EOSYNOPSIS
14+
);
15+
$args->parseStandardArguments();
16+
17+
$workflows = id(new PhutilClassMapQuery())
18+
->setAncestorClass('PhabricatorLockManagementWorkflow')
19+
->execute();
20+
$workflows[] = new PhutilHelpArgumentWorkflow();
21+
$args->parseWorkflows($workflows);

‎src/__phutil_library_map__.php

+8
Original file line numberDiff line numberDiff line change
@@ -2670,6 +2670,8 @@
26702670
'PhabricatorDaemonController' => 'applications/daemon/controller/PhabricatorDaemonController.php',
26712671
'PhabricatorDaemonDAO' => 'applications/daemon/storage/PhabricatorDaemonDAO.php',
26722672
'PhabricatorDaemonEventListener' => 'applications/daemon/event/PhabricatorDaemonEventListener.php',
2673+
'PhabricatorDaemonLockLog' => 'applications/daemon/storage/PhabricatorDaemonLockLog.php',
2674+
'PhabricatorDaemonLockLogGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php',
26732675
'PhabricatorDaemonLog' => 'applications/daemon/storage/PhabricatorDaemonLog.php',
26742676
'PhabricatorDaemonLogEvent' => 'applications/daemon/storage/PhabricatorDaemonLogEvent.php',
26752677
'PhabricatorDaemonLogEventGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLogEventGarbageCollector.php',
@@ -3204,6 +3206,8 @@
32043206
'PhabricatorLocalTimeTestCase' => 'view/__tests__/PhabricatorLocalTimeTestCase.php',
32053207
'PhabricatorLocaleScopeGuard' => 'infrastructure/internationalization/scope/PhabricatorLocaleScopeGuard.php',
32063208
'PhabricatorLocaleScopeGuardTestCase' => 'infrastructure/internationalization/scope/__tests__/PhabricatorLocaleScopeGuardTestCase.php',
3209+
'PhabricatorLockLogManagementWorkflow' => 'applications/daemon/management/PhabricatorLockLogManagementWorkflow.php',
3210+
'PhabricatorLockManagementWorkflow' => 'applications/daemon/management/PhabricatorLockManagementWorkflow.php',
32073211
'PhabricatorLogTriggerAction' => 'infrastructure/daemon/workers/action/PhabricatorLogTriggerAction.php',
32083212
'PhabricatorLogoutController' => 'applications/auth/controller/PhabricatorLogoutController.php',
32093213
'PhabricatorLunarPhasePolicyRule' => 'applications/policy/rule/PhabricatorLunarPhasePolicyRule.php',
@@ -8194,6 +8198,8 @@
81948198
'PhabricatorDaemonController' => 'PhabricatorController',
81958199
'PhabricatorDaemonDAO' => 'PhabricatorLiskDAO',
81968200
'PhabricatorDaemonEventListener' => 'PhabricatorEventListener',
8201+
'PhabricatorDaemonLockLog' => 'PhabricatorDaemonDAO',
8202+
'PhabricatorDaemonLockLogGarbageCollector' => 'PhabricatorGarbageCollector',
81978203
'PhabricatorDaemonLog' => array(
81988204
'PhabricatorDaemonDAO',
81998205
'PhabricatorPolicyInterface',
@@ -8794,6 +8800,8 @@
87948800
'PhabricatorLocalTimeTestCase' => 'PhabricatorTestCase',
87958801
'PhabricatorLocaleScopeGuard' => 'Phobject',
87968802
'PhabricatorLocaleScopeGuardTestCase' => 'PhabricatorTestCase',
8803+
'PhabricatorLockLogManagementWorkflow' => 'PhabricatorLockManagementWorkflow',
8804+
'PhabricatorLockManagementWorkflow' => 'PhabricatorManagementWorkflow',
87978805
'PhabricatorLogTriggerAction' => 'PhabricatorTriggerAction',
87988806
'PhabricatorLogoutController' => 'PhabricatorAuthController',
87998807
'PhabricatorLunarPhasePolicyRule' => 'PhabricatorPolicyRule',

‎src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<?php
2+
3+
final class PhabricatorDaemonLockLogGarbageCollector
4+
extends PhabricatorGarbageCollector {
5+
6+
const COLLECTORCONST = 'daemon.lock-log';
7+
8+
public function getCollectorName() {
9+
return pht('Lock Logs');
10+
}
11+
12+
public function getDefaultRetentionPolicy() {
13+
return 0;
14+
}
15+
16+
protected function collectGarbage() {
17+
$table = new PhabricatorDaemonLockLog();
18+
$conn = $table->establishConnection('w');
19+
20+
queryfx(
21+
$conn,
22+
'DELETE FROM %T WHERE dateCreated < %d LIMIT 100',
23+
$table->getTableName(),
24+
$this->getGarbageEpoch());
25+
26+
return ($conn->getAffectedRows() == 100);
27+
}
28+
29+
}

‎src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php

+222
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
<?php
2+
3+
final class PhabricatorLockLogManagementWorkflow
4+
extends PhabricatorLockManagementWorkflow {
5+
6+
protected function didConstruct() {
7+
$this
8+
->setName('log')
9+
->setSynopsis(pht('Enable, disable, or show the lock log.'))
10+
->setArguments(
11+
array(
12+
array(
13+
'name' => 'enable',
14+
'help' => pht('Enable the lock log.'),
15+
),
16+
array(
17+
'name' => 'disable',
18+
'help' => pht('Disable the lock log.'),
19+
),
20+
array(
21+
'name' => 'name',
22+
'param' => 'name',
23+
'help' => pht('Review logs for a specific lock.'),
24+
),
25+
));
26+
}
27+
28+
public function execute(PhutilArgumentParser $args) {
29+
$is_enable = $args->getArg('enable');
30+
$is_disable = $args->getArg('disable');
31+
32+
if ($is_enable && $is_disable) {
33+
throw new PhutilArgumentUsageException(
34+
pht(
35+
'You can not both "--enable" and "--disable" the lock log.'));
36+
}
37+
38+
$with_name = $args->getArg('name');
39+
40+
if ($is_enable || $is_disable) {
41+
if (strlen($with_name)) {
42+
throw new PhutilArgumentUsageException(
43+
pht(
44+
'You can not both "--enable" or "--disable" with search '.
45+
'parameters like "--name".'));
46+
}
47+
48+
$gc = new PhabricatorDaemonLockLogGarbageCollector();
49+
$is_enabled = (bool)$gc->getRetentionPolicy();
50+
51+
$config_key = 'phd.garbage-collection';
52+
$const = $gc->getCollectorConstant();
53+
$value = PhabricatorEnv::getEnvConfig($config_key);
54+
55+
if ($is_disable) {
56+
if (!$is_enabled) {
57+
echo tsprintf(
58+
"%s\n",
59+
pht('Lock log is already disabled.'));
60+
return 0;
61+
}
62+
echo tsprintf(
63+
"%s\n",
64+
pht('Disabling the lock log.'));
65+
66+
unset($value[$const]);
67+
} else {
68+
if ($is_enabled) {
69+
echo tsprintf(
70+
"%s\n",
71+
pht('Lock log is already enabled.'));
72+
return 0;
73+
}
74+
echo tsprintf(
75+
"%s\n",
76+
pht('Enabling the lock log.'));
77+
78+
$value[$const] = phutil_units('24 hours in seconds');
79+
}
80+
81+
id(new PhabricatorConfigLocalSource())
82+
->setKeys(
83+
array(
84+
$config_key => $value,
85+
));
86+
87+
echo tsprintf(
88+
"%s\n",
89+
pht('Done.'));
90+
91+
echo tsprintf(
92+
"%s\n",
93+
pht('Restart daemons to apply changes.'));
94+
95+
return 0;
96+
}
97+
98+
$table = new PhabricatorDaemonLockLog();
99+
$conn = $table->establishConnection('r');
100+
101+
$parts = array();
102+
if (strlen($with_name)) {
103+
$parts[] = qsprintf(
104+
$conn,
105+
'lockName = %s',
106+
$with_name);
107+
}
108+
109+
if (!$parts) {
110+
$constraint = '1 = 1';
111+
} else {
112+
$constraint = '('.implode(') AND (', $parts).')';
113+
}
114+
115+
$logs = $table->loadAllWhere(
116+
'%Q ORDER BY id DESC LIMIT 100',
117+
$constraint);
118+
$logs = array_reverse($logs);
119+
120+
if (!$logs) {
121+
echo tsprintf(
122+
"%s\n",
123+
pht('No matching lock logs.'));
124+
return 0;
125+
}
126+
127+
$table = id(new PhutilConsoleTable())
128+
->setBorders(true)
129+
->addColumn(
130+
'id',
131+
array(
132+
'title' => pht('Lock'),
133+
))
134+
->addColumn(
135+
'name',
136+
array(
137+
'title' => pht('Name'),
138+
))
139+
->addColumn(
140+
'acquired',
141+
array(
142+
'title' => pht('Acquired'),
143+
))
144+
->addColumn(
145+
'released',
146+
array(
147+
'title' => pht('Released'),
148+
))
149+
->addColumn(
150+
'held',
151+
array(
152+
'title' => pht('Held'),
153+
))
154+
->addColumn(
155+
'parameters',
156+
array(
157+
'title' => pht('Parameters'),
158+
))
159+
->addColumn(
160+
'context',
161+
array(
162+
'title' => pht('Context'),
163+
));
164+
165+
$viewer = $this->getViewer();
166+
167+
foreach ($logs as $log) {
168+
$created = $log->getDateCreated();
169+
$released = $log->getLockReleased();
170+
171+
if ($released) {
172+
$held = '+'.($released - $created);
173+
} else {
174+
$held = null;
175+
}
176+
177+
$created = phabricator_datetime($created, $viewer);
178+
$released = phabricator_datetime($released, $viewer);
179+
180+
$parameters = $log->getLockParameters();
181+
$context = $log->getLockContext();
182+
183+
$table->addRow(
184+
array(
185+
'id' => $log->getID(),
186+
'name' => $log->getLockName(),
187+
'acquired' => $created,
188+
'released' => $released,
189+
'held' => $held,
190+
'parameters' => $this->flattenParameters($parameters),
191+
'context' => $this->flattenParameters($context),
192+
));
193+
}
194+
195+
$table->draw();
196+
197+
return 0;
198+
}
199+
200+
private function flattenParameters(array $params, $keys = true) {
201+
$flat = array();
202+
foreach ($params as $key => $value) {
203+
if (is_array($value)) {
204+
$value = $this->flattenParameters($value, false);
205+
}
206+
if ($keys) {
207+
$flat[] = "{$key}={$value}";
208+
} else {
209+
$flat[] = "{$value}";
210+
}
211+
}
212+
213+
if ($keys) {
214+
$flat = implode(', ', $flat);
215+
} else {
216+
$flat = implode(' ', $flat);
217+
}
218+
219+
return $flat;
220+
}
221+
222+
}

‎src/applications/daemon/management/PhabricatorLockManagementWorkflow.php

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?php
2+
3+
abstract class PhabricatorLockManagementWorkflow
4+
extends PhabricatorManagementWorkflow {}

‎src/applications/daemon/storage/PhabricatorDaemonLockLog.php

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
final class PhabricatorDaemonLockLog
4+
extends PhabricatorDaemonDAO {
5+
6+
protected $lockName;
7+
protected $lockReleased;
8+
protected $lockParameters = array();
9+
protected $lockContext = array();
10+
11+
protected function getConfiguration() {
12+
return array(
13+
self::CONFIG_SERIALIZATION => array(
14+
'lockParameters' => self::SERIALIZATION_JSON,
15+
'lockContext' => self::SERIALIZATION_JSON,
16+
),
17+
self::CONFIG_COLUMN_SCHEMA => array(
18+
'lockName' => 'text64',
19+
'lockReleased' => 'epoch?',
20+
),
21+
self::CONFIG_KEY_SCHEMA => array(
22+
'key_lock' => array(
23+
'columns' => array('lockName'),
24+
),
25+
'key_created' => array(
26+
'columns' => array('dateCreated'),
27+
),
28+
),
29+
) + parent::getConfiguration();
30+
}
31+
32+
}

‎src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php

+4-2
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,10 @@ final public function runCollector() {
100100

101101
// Hold a lock while performing collection to avoid racing other daemons
102102
// running the same collectors.
103-
$lock_name = 'gc:'.$this->getCollectorConstant();
104-
$lock = PhabricatorGlobalLock::newLock($lock_name);
103+
$params = array(
104+
'collector' => $this->getCollectorConstant(),
105+
);
106+
$lock = PhabricatorGlobalLock::newLock('gc', $params);
105107

106108
try {
107109
$lock->lock(5);

‎src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php

+8-4
Original file line numberDiff line numberDiff line change
@@ -1163,12 +1163,16 @@ final protected function lock(PhabricatorStorageManagementAPI $api) {
11631163
// Although we're holding this lock on different databases so it could
11641164
// have the same name on each as far as the database is concerned, the
11651165
// locks would be the same within this process.
1166-
$ref_key = $api->getRef()->getRefKey();
1167-
$ref_hash = PhabricatorHash::digestForIndex($ref_key);
1168-
$lock_name = 'adjust('.$ref_hash.')';
1166+
$parameters = array(
1167+
'refKey' => $api->getRef()->getRefKey(),
1168+
);
1169+
1170+
// We disable logging for this lock because we may not have created the
1171+
// log table yet, or may need to adjust it.
11691172

1170-
return PhabricatorGlobalLock::newLock($lock_name)
1173+
return PhabricatorGlobalLock::newLock('adjust', $parameters)
11711174
->useSpecificConnection($api->getConn(null))
1175+
->setDisableLogging(true)
11721176
->lock();
11731177
}
11741178

0 commit comments

Comments
 (0)
Failed to load comments.