diff --git a/src/applications/config/check/PhabricatorDaemonsSetupCheck.php b/src/applications/config/check/PhabricatorDaemonsSetupCheck.php index 331a4f0908..7ef44dc384 100644 --- a/src/applications/config/check/PhabricatorDaemonsSetupCheck.php +++ b/src/applications/config/check/PhabricatorDaemonsSetupCheck.php @@ -8,14 +8,21 @@ final class PhabricatorDaemonsSetupCheck extends PhabricatorSetupCheck { protected function executeChecks() { - $task_daemon = id(new PhabricatorDaemonLogQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE) - ->withDaemonClasses(array('PhabricatorTaskmasterDaemon')) - ->setLimit(1) - ->execute(); + try { + $task_daemons = id(new PhabricatorDaemonLogQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE) + ->withDaemonClasses(array('PhabricatorTaskmasterDaemon')) + ->setLimit(1) + ->execute(); - if (!$task_daemon) { + $no_daemons = !$task_daemons; + } catch (Exception $ex) { + // Just skip this warning if the query fails for some reason. + $no_daemons = false; + } + + if ($no_daemons) { $doc_href = PhabricatorEnv::getDoclink('Managing Daemons with phd'); $summary = pht( diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index d863c928b7..1de39f3468 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -322,6 +322,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'directly supported. Prefixes and other strings may be customized with '. '"translation.override".'); + $phd_reason = pht( + 'Use "bin/phd debug ..." to get a detailed daemon execution log.'); + $ancient_config += array( 'phid.external-loaders' => pht( @@ -539,6 +542,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'phd.pid-directory' => pht( 'Phabricator daemons no longer use PID files.'), + + 'phd.trace' => $phd_reason, + 'phd.verbose' => $phd_reason, ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorPHDConfigOptions.php b/src/applications/config/option/PhabricatorPHDConfigOptions.php index 7a1d39e617..259660d562 100644 --- a/src/applications/config/option/PhabricatorPHDConfigOptions.php +++ b/src/applications/config/option/PhabricatorPHDConfigOptions.php @@ -43,22 +43,6 @@ final class PhabricatorPHDConfigOptions "configuration changes are picked up by the daemons ". "automatically, but pool sizes can not be changed without a ". "restart.")), - $this->newOption('phd.verbose', 'bool', false) - ->setLocked(true) - ->setBoolOptions( - array( - pht('Verbose mode'), - pht('Normal mode'), - )) - ->setSummary(pht("Launch daemons in 'verbose' mode by default.")) - ->setDescription( - pht( - "Launch daemons in 'verbose' mode by default. This creates a lot ". - "of output, but can help debug issues. Daemons launched in debug ". - "mode with '%s' are always launched in verbose mode. ". - "See also '%s'.", - 'phd debug', - 'phd.trace')), $this->newOption('phd.user', 'string', null) ->setLocked(true) ->setSummary(pht('System user to run daemons as.')) @@ -68,22 +52,6 @@ final class PhabricatorPHDConfigOptions 'user will own the working copies of any repositories that '. 'Phabricator imports or manages. This option is new and '. 'experimental.')), - $this->newOption('phd.trace', 'bool', false) - ->setLocked(true) - ->setBoolOptions( - array( - pht('Trace mode'), - pht('Normal mode'), - )) - ->setSummary(pht("Launch daemons in 'trace' mode by default.")) - ->setDescription( - pht( - "Launch daemons in 'trace' mode by default. This creates an ". - "ENORMOUS amount of output, but can help debug issues. Daemons ". - "launched in debug mode with '%s' are always launched in ". - "trace mode. See also '%s'.", - 'phd debug', - 'phd.verbose')), $this->newOption('phd.garbage-collection', 'wild', array()) ->setLocked(true) ->setLockedMessage( diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php index 05d94e218d..b9645323c2 100644 --- a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php +++ b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php @@ -116,11 +116,11 @@ abstract class PhabricatorDaemonManagementWorkflow $trace = PhutilArgumentParser::isTraceModeEnabled(); $flags = array(); - if ($trace || PhabricatorEnv::getEnvConfig('phd.trace')) { + if ($trace) { $flags[] = '--trace'; } - if ($debug || PhabricatorEnv::getEnvConfig('phd.verbose')) { + if ($debug) { $flags[] = '--verbose'; } diff --git a/src/infrastructure/daemon/PhutilDaemonHandle.php b/src/infrastructure/daemon/PhutilDaemonHandle.php index 5030f5330c..428a64a056 100644 --- a/src/infrastructure/daemon/PhutilDaemonHandle.php +++ b/src/infrastructure/daemon/PhutilDaemonHandle.php @@ -16,7 +16,6 @@ final class PhutilDaemonHandle extends Phobject { private $restartAt; private $busyEpoch; - private $pid; private $daemonID; private $deadline; private $heartbeat; @@ -104,7 +103,7 @@ final class PhutilDaemonHandle extends Phobject { } public function isRunning() { - return (bool)$this->future; + return (bool)$this->getFuture(); } public function isHibernating() { @@ -134,10 +133,6 @@ final class PhutilDaemonHandle extends Phobject { return (!$this->shouldRestart && !$this->isRunning()); } - public function getFuture() { - return $this->future; - } - public function update() { if (!$this->isRunning()) { if (!$this->shouldRestart) { @@ -152,11 +147,19 @@ final class PhutilDaemonHandle extends Phobject { $this->startDaemonProcess(); } - $future = $this->future; + $future = $this->getFuture(); $result = null; - if ($future->isReady()) { - $result = $future->resolve(); + $caught = null; + if ($future->canResolve()) { + $this->future = null; + try { + $result = $future->resolve(); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } } list($stdout, $stderr) = $future->read(); @@ -173,16 +176,22 @@ final class PhutilDaemonHandle extends Phobject { } } - if ($result !== null) { - list($err) = $result; + if ($result !== null || $caught !== null) { - if ($err) { - $this->logMessage('FAIL', pht('Process exited with error %s.', $err)); + if ($caught) { + $message = pht( + 'Process failed with exception: %s', + $caught->getMessage()); + $this->logMessage('FAIL', $message); } else { - $this->logMessage('DONE', pht('Process exited normally.')); - } + list($err) = $result; - $this->future = null; + if ($err) { + $this->logMessage('FAIL', pht('Process exited with error %s.', $err)); + } else { + $this->logMessage('DONE', pht('Process exited normally.')); + } + } if ($this->shouldShutdown) { $this->restartAt = null; @@ -244,8 +253,22 @@ final class PhutilDaemonHandle extends Phobject { return $this->daemonID; } - public function getPID() { - return $this->pid; + private function getFuture() { + return $this->future; + } + + private function getPID() { + $future = $this->getFuture(); + + if (!$future) { + return null; + } + + if (!$future->hasPID()) { + return null; + } + + return $future->getPID(); } private function getCaptureBufferSize() { @@ -327,13 +350,14 @@ final class PhutilDaemonHandle extends Phobject { private function annihilateProcessGroup() { $pid = $this->getPID(); - - $pgid = posix_getpgid($pid); - if ($pid && $pgid) { - posix_kill(-$pgid, SIGTERM); - sleep($this->getKillDelay()); - posix_kill(-$pgid, SIGKILL); - $this->pid = null; + if ($pid) { + $pgid = posix_getpgid($pid); + if ($pgid) { + posix_kill(-$pgid, SIGTERM); + sleep($this->getKillDelay()); + posix_kill(-$pgid, SIGKILL); + $this->pid = null; + } } } @@ -345,10 +369,12 @@ final class PhutilDaemonHandle extends Phobject { $this->stdoutBuffer = ''; $this->hibernating = false; - $this->future = $this->newExecFuture(); - $this->future->start(); + $future = $this->newExecFuture(); + $this->future = $future; - $this->pid = $this->future->getPID(); + $pool = $this->getDaemonPool(); + $overseer = $pool->getOverseer(); + $overseer->addFutureToPool($future); } private function didReadStdout($data) { @@ -440,7 +466,10 @@ final class PhutilDaemonHandle extends Phobject { // naturally be restarted after it exits, as though it had exited after an // unhandled exception. - posix_kill($this->getPID(), SIGINT); + $pid = $this->getPID(); + if ($pid) { + posix_kill($pid, SIGINT); + } } public function didReceiveGracefulSignal($signo) { @@ -461,7 +490,10 @@ final class PhutilDaemonHandle extends Phobject { $this->logMessage('DONE', $sigmsg, $signo); - posix_kill($this->getPID(), SIGINT); + $pid = $this->getPID(); + if ($pid) { + posix_kill($pid, SIGINT); + } } public function didReceiveTerminateSignal($signo) { diff --git a/src/infrastructure/daemon/PhutilDaemonOverseer.php b/src/infrastructure/daemon/PhutilDaemonOverseer.php index 77f6f6a20a..974a9cf3ed 100644 --- a/src/infrastructure/daemon/PhutilDaemonOverseer.php +++ b/src/infrastructure/daemon/PhutilDaemonOverseer.php @@ -181,10 +181,6 @@ EOHELP } } - foreach ($pool->getFutures() as $future) { - $future_pool->addFuture($future); - } - if ($pool->getDaemons()) { $running_pools = true; } @@ -210,6 +206,11 @@ EOHELP exit($this->err); } + public function addFutureToPool(Future $future) { + $this->getFuturePool()->addFuture($future); + return $this; + } + private function getFuturePool() { if (!$this->futurePool) { $pool = new FuturePool(); diff --git a/src/infrastructure/daemon/PhutilDaemonPool.php b/src/infrastructure/daemon/PhutilDaemonPool.php index 50b22289a8..9cdc15228e 100644 --- a/src/infrastructure/daemon/PhutilDaemonPool.php +++ b/src/infrastructure/daemon/PhutilDaemonPool.php @@ -111,18 +111,6 @@ final class PhutilDaemonPool extends Phobject { return $this->daemons; } - public function getFutures() { - $futures = array(); - foreach ($this->getDaemons() as $daemon) { - $future = $daemon->getFuture(); - if ($future) { - $futures[] = $future; - } - } - - return $futures; - } - public function didReceiveSignal($signal, $signo) { switch ($signal) { case PhutilDaemonOverseer::SIGNAL_GRACEFUL: diff --git a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php index c2276843db..12b06131d8 100644 --- a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php +++ b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php @@ -24,7 +24,7 @@ final class PhabricatorTaskmasterDaemon extends PhabricatorDaemon { if ($ex) { if ($ex instanceof PhabricatorWorkerPermanentFailureException) { // NOTE: Make sure these reach the daemon log, even when not - // running in "phd.verbose" mode. See T12803 for discussion. + // running in verbose mode. See T12803 for discussion. $log_exception = new PhutilProxyException( pht( 'Task "%s" encountered a permanent failure and was '. diff --git a/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php b/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php index a5494aa05f..b8bee73961 100644 --- a/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php +++ b/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php @@ -142,22 +142,9 @@ final class PhutilProseDifferenceEngine extends Phobject { } if ($level < 2) { - // Split pieces into separate text and whitespace sections: make one - // piece out of all the whitespace at the beginning, one piece out of - // all the actual text in the middle, and one piece out of all the - // whitespace at the end. - - $matches = null; - preg_match('/^(\s*)(.*?)(\s*)\z/s', $result, $matches); - - if (strlen($matches[1])) { - $results[] = $matches[1]; - } - if (strlen($matches[2])) { - $results[] = $matches[2]; - } - if (strlen($matches[3])) { - $results[] = $matches[3]; + $trimmed_pieces = $this->trimApart($result); + foreach ($trimmed_pieces as $trimmed_piece) { + $results[] = $trimmed_piece; } } else { $results[] = $result; @@ -272,4 +259,36 @@ final class PhutilProseDifferenceEngine extends Phobject { return $blocks; } + public static function trimApart($input) { + // Split pieces into separate text and whitespace sections: make one + // piece out of all the whitespace at the beginning, one piece out of + // all the actual text in the middle, and one piece out of all the + // whitespace at the end. + + $parts = array(); + + $length = strlen($input); + + $corpus = ltrim($input); + $l_length = strlen($corpus); + if ($l_length !== $length) { + $parts[] = substr($input, 0, $length - $l_length); + } + + $corpus = rtrim($corpus); + $lr_length = strlen($corpus); + + if ($lr_length) { + $parts[] = $corpus; + } + + if ($lr_length !== $l_length) { + // NOTE: This will be a negative value; we're slicing from the end of + // the input string. + $parts[] = substr($input, $lr_length - $l_length); + } + + return $parts; + } + } diff --git a/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php b/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php index 823fef650a..d2e11cac5b 100644 --- a/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php +++ b/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php @@ -3,6 +3,39 @@ final class PhutilProseDiffTestCase extends PhabricatorTestCase { + public function testTrimApart() { + $map = array( + '' => array(), + 'a' => array('a'), + ' a ' => array( + ' ', + 'a', + ' ', + ), + ' a' => array( + ' ', + 'a', + ), + 'a ' => array( + 'a', + ' ', + ), + ' a b ' => array( + ' ', + 'a b', + ' ', + ), + ); + + foreach ($map as $input => $expect) { + $actual = PhutilProseDifferenceEngine::trimApart($input); + $this->assertEqual( + $expect, + $actual, + pht('Trim Apart: %s', $input)); + } + } + public function testProseDiffsDistance() { $this->assertProseParts( '',