Skip to content

Commit e94f6e7

Browse files
author
epriestley
committed
Formalize and centralize signal handling in libphutil scripts
Summary: Ref T11592. By default, when PHP receives a SIGTERM, it just exits without running destructors. This makes it difficult for us to release locks, free resources, terminate subprocesses, etc. We already fix this behavior in the daemon wrapper, but I want to extend this to all libphutil scripts so that everything terminates subprocesses correctly. Since `pcntl_signal()` can only accept one signal handler, that means we need to centralize signal handling. This creates a centralized signal router and some formal signal handlers, and always installs basic routing behavior for SIGHUP (route, plus dump backtraces) and SIGTERM (exit, but run destructors). Test Plan: Used this test script: ``` <?php require_once '../libphutil/scripts/__init_script__.php'; echo getmypid()."\n"; $future = new ExecFuture('sleep 240'); $future->resolve(); ``` - Sent it SIGTERM. - Before patch: `sleep` subprocess still alive. - After patch: `sleep` subprocess killed. - Sent it SIGHUP. - Verified it dumped a trace properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11592 Differential Revision: https://secure.phabricator.com/D16504
1 parent b00eff5 commit e94f6e7

7 files changed

+153
-26
lines changed

scripts/__init_script__.php

+5-15
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
<?php
22

3+
declare(ticks = 1);
4+
35
function __phutil_init_script__() {
46
// Adjust the runtime language configuration to be reasonable and inline with
57
// expectations. We do this first, then load libraries.
@@ -80,22 +82,10 @@ function __phutil_init_script__() {
8082
require_once $root.'/src/__phutil_library_init__.php';
8183

8284
PhutilErrorHandler::initialize();
85+
$router = PhutilSignalRouter::initialize();
8386

84-
// If possible, install a signal handler for SIGHUP which prints the current
85-
// backtrace out to a named file. This is particularly helpful in debugging
86-
// hung/spinning processes.
87-
if (function_exists('pcntl_signal')) {
88-
pcntl_signal(SIGHUP, '__phutil_signal_handler__');
89-
}
90-
}
91-
92-
function __phutil_signal_handler__($signal_number) {
93-
$e = new Exception();
94-
$pid = getmypid();
95-
// Some Phabricator daemons may not be attached to a terminal.
96-
Filesystem::writeFile(
97-
sys_get_temp_dir().'/phabricator_backtrace_'.$pid,
98-
$e->getTraceAsString());
87+
$handler = new PhutilBacktraceSignalHandler();
88+
$router->installHandler('phutil.backtrace', $handler);
9989
}
10090

10191
__phutil_init_script__();

src/__phutil_library_map__.php

+8
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@
119119
'PhutilAuthCredentialException' => 'auth/exception/PhutilAuthCredentialException.php',
120120
'PhutilAuthException' => 'auth/exception/PhutilAuthException.php',
121121
'PhutilAuthUserAbortedException' => 'auth/exception/PhutilAuthUserAbortedException.php',
122+
'PhutilBacktraceSignalHandler' => 'future/exec/PhutilBacktraceSignalHandler.php',
122123
'PhutilBallOfPHP' => 'phage/util/PhutilBallOfPHP.php',
123124
'PhutilBitbucketAuthAdapter' => 'auth/PhutilBitbucketAuthAdapter.php',
124125
'PhutilBootloader' => 'moduleutils/PhutilBootloader.php',
@@ -132,6 +133,7 @@
132133
'PhutilCIDRList' => 'ip/PhutilCIDRList.php',
133134
'PhutilCLikeCodeSnippetContextFreeGrammar' => 'grammar/code/PhutilCLikeCodeSnippetContextFreeGrammar.php',
134135
'PhutilCallbackFilterIterator' => 'utils/PhutilCallbackFilterIterator.php',
136+
'PhutilCallbackSignalHandler' => 'future/exec/PhutilCallbackSignalHandler.php',
135137
'PhutilChannel' => 'channel/PhutilChannel.php',
136138
'PhutilChannelChannel' => 'channel/PhutilChannelChannel.php',
137139
'PhutilChannelTestCase' => 'channel/__tests__/PhutilChannelTestCase.php',
@@ -358,6 +360,8 @@
358360
'PhutilServiceProfiler' => 'serviceprofiler/PhutilServiceProfiler.php',
359361
'PhutilShellLexer' => 'lexer/PhutilShellLexer.php',
360362
'PhutilShellLexerTestCase' => 'lexer/__tests__/PhutilShellLexerTestCase.php',
363+
'PhutilSignalHandler' => 'future/exec/PhutilSignalHandler.php',
364+
'PhutilSignalRouter' => 'future/exec/PhutilSignalRouter.php',
361365
'PhutilSimpleOptions' => 'parser/PhutilSimpleOptions.php',
362366
'PhutilSimpleOptionsLexer' => 'lexer/PhutilSimpleOptionsLexer.php',
363367
'PhutilSimpleOptionsLexerTestCase' => 'lexer/__tests__/PhutilSimpleOptionsLexerTestCase.php',
@@ -680,6 +684,7 @@
680684
'PhutilAuthCredentialException' => 'PhutilAuthException',
681685
'PhutilAuthException' => 'Exception',
682686
'PhutilAuthUserAbortedException' => 'PhutilAuthException',
687+
'PhutilBacktraceSignalHandler' => 'PhutilSignalHandler',
683688
'PhutilBallOfPHP' => 'Phobject',
684689
'PhutilBitbucketAuthAdapter' => 'PhutilOAuth1AuthAdapter',
685690
'PhutilBootloaderException' => 'Exception',
@@ -695,6 +700,7 @@
695700
'PhutilCIDRList' => 'Phobject',
696701
'PhutilCLikeCodeSnippetContextFreeGrammar' => 'PhutilCodeSnippetContextFreeGrammar',
697702
'PhutilCallbackFilterIterator' => 'FilterIterator',
703+
'PhutilCallbackSignalHandler' => 'PhutilSignalHandler',
698704
'PhutilChannel' => 'Phobject',
699705
'PhutilChannelChannel' => 'PhutilChannel',
700706
'PhutilChannelTestCase' => 'PhutilTestCase',
@@ -930,6 +936,8 @@
930936
'PhutilServiceProfiler' => 'Phobject',
931937
'PhutilShellLexer' => 'PhutilLexer',
932938
'PhutilShellLexerTestCase' => 'PhutilTestCase',
939+
'PhutilSignalHandler' => 'Phobject',
940+
'PhutilSignalRouter' => 'Phobject',
933941
'PhutilSimpleOptions' => 'Phobject',
934942
'PhutilSimpleOptionsLexer' => 'PhutilLexer',
935943
'PhutilSimpleOptionsLexerTestCase' => 'PhutilTestCase',

src/daemon/PhutilDaemon.php

+8-11
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,16 @@ final public function getVerbose() {
7070
return $this->verbose;
7171
}
7272

73-
private static $sighandlerInstalled;
74-
7573
final public function __construct(array $argv) {
7674
$this->argv = $argv;
7775

78-
if (!self::$sighandlerInstalled) {
79-
self::$sighandlerInstalled = true;
80-
pcntl_signal(SIGTERM, __CLASS__.'::exitOnSignal');
76+
$router = PhutilSignalRouter::getRouter();
77+
$handler_key = 'daemon.term';
78+
if (!$router->getHandler($handler_key)) {
79+
$handler = new PhutilCallbackSignalHandler(
80+
SIGTERM,
81+
__CLASS__.'::onTermSignal');
82+
$router->installHandler($handler_key, $handler);
8183
}
8284

8385
pcntl_signal(SIGINT, array($this, 'onGracefulSignal'));
@@ -166,13 +168,8 @@ protected function willSleep($duration) {
166168
return;
167169
}
168170

169-
public static function exitOnSignal($signo) {
171+
public static function onTermSignal($signo) {
170172
self::didCatchSignal($signo);
171-
172-
// Normally, PHP doesn't invoke destructors when exiting in response to
173-
// a signal. This forces it to do so, so we have a fighting chance of
174-
// releasing any locks, leases or resources on our way out.
175-
exit(128 + $signo);
176173
}
177174

178175
final protected function getArgv() {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
/**
4+
* Signal handler for SIGHUP which prints the current backtrace out to a
5+
* file. This is particularly helpful in debugging hung/spinning processes.
6+
*/
7+
final class PhutilBacktraceSignalHandler extends PhutilSignalHandler {
8+
9+
public function canHandleSignal(PhutilSignalRouter $router, $signo) {
10+
return ($signo === SIGHUP);
11+
}
12+
13+
public function handleSignal(PhutilSignalRouter $router, $signo) {
14+
$e = new Exception();
15+
$pid = getmypid();
16+
// Some Phabricator daemons may not be attached to a terminal.
17+
Filesystem::writeFile(
18+
sys_get_temp_dir().'/phabricator_backtrace_'.$pid,
19+
$e->getTraceAsString());
20+
}
21+
22+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
4+
final class PhutilCallbackSignalHandler extends PhutilSignalHandler {
5+
6+
private $signal;
7+
private $callback;
8+
9+
public function __construct($signal, $callback) {
10+
$this->signal = $signal;
11+
$this->callback = $callback;
12+
}
13+
14+
public function canHandleSignal(PhutilSignalRouter $router, $signo) {
15+
return ($signo === $this->signal);
16+
}
17+
18+
public function handleSignal(PhutilSignalRouter $router, $signo) {
19+
call_user_func($this->callback, $signo);
20+
}
21+
22+
}
+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?php
2+
3+
abstract class PhutilSignalHandler extends Phobject {
4+
5+
abstract public function canHandleSignal(PhutilSignalRouter $router, $signo);
6+
abstract public function handleSignal(PhutilSignalRouter $router, $signo);
7+
8+
}
+80
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
<?php
2+
3+
final class PhutilSignalRouter extends Phobject {
4+
5+
private $handlers = array();
6+
private static $router;
7+
8+
private function __construct() {
9+
// <private>
10+
}
11+
12+
public static function initialize() {
13+
if (!self::$router) {
14+
$router = new self();
15+
16+
pcntl_signal(SIGHUP, array($router, 'routeSignal'));
17+
pcntl_signal(SIGTERM, array($router, 'routeSignal'));
18+
19+
self::$router = $router;
20+
}
21+
22+
return self::getRouter();
23+
}
24+
25+
public static function getRouter() {
26+
if (!self::$router) {
27+
throw new Exception(pht('Signal router has not been initialized!'));
28+
}
29+
30+
return self::$router;
31+
}
32+
33+
public function installHandler($key, PhutilSignalHandler $handler) {
34+
if (isset($this->handlers[$key])) {
35+
throw new Exception(
36+
pht(
37+
'Signal handler with key "%s" is already installed.',
38+
$key));
39+
}
40+
41+
$this->handlers[$key] = $handler;
42+
43+
return $this;
44+
}
45+
46+
public function getHandler($key) {
47+
return idx($this->handlers, $key);
48+
}
49+
50+
public function routeSignal($signo) {
51+
$exceptions = array();
52+
53+
$handlers = $this->handlers;
54+
foreach ($handlers as $key => $handler) {
55+
try {
56+
if ($handler->canHandleSignal($this, $signo)) {
57+
$handler->handleSignal($this, $signo);
58+
}
59+
} catch (Exception $ex) {
60+
$exceptions[] = $ex;
61+
}
62+
}
63+
64+
if ($exceptions) {
65+
throw new PhutilAggregateException(
66+
pht(
67+
'Signal handlers raised exceptions while handling "%s".',
68+
phutil_get_signal_name($signo)));
69+
}
70+
71+
switch ($signo) {
72+
case SIGTERM:
73+
// Normally, PHP doesn't invoke destructors when exiting in response to
74+
// a signal. This forces it to do so, so we have a fighting chance of
75+
// releasing any locks, leases or resources on our way out.
76+
exit(128 + $signo);
77+
}
78+
}
79+
80+
}

0 commit comments

Comments
 (0)