Formalize and centralize signal handling in libphutil scripts
ClosedPublic
Actions

Authored by epriestley on Sep 7 2016, 1:01 AM.

Details

Reviewers

chad

Maniphest Tasks

T11592: `repository update` may be killed after timing out without annihilating `git fetch` subprocesses

Commits

rPHUe94f6e738e2d: 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.

Diff Detail

Repository

rPHU libphutil

Branch

signal1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 13598
Build 17517: Run Core Tests
Build 17516: arc lint + arc unit

Event Timeline

epriestley updated this revision to Diff 39718.Sep 7 2016, 1:01 AM

epriestley retitled this revision from to Formalize and centralize signal handling in libphutil scripts.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T11592: `repository update` may be killed after timing out without annihilating `git fetch` subprocesses.

Note that this doesn't actually solve the issue in T11592, it just makes us respond to SIGTERM better. We still send SIGKILL instead of SIGTERM. The next change will switch us to SIGTERM + wait + SIGKILL.

chad accepted this revision.Sep 7 2016, 1:14 AM

chad edited edge metadata.

This revision is now accepted and ready to land.Sep 7 2016, 1:14 AM

Closed by commit rPHUe94f6e738e2d: Formalize and centralize signal handling in libphutil scripts (authored by epriestley, committed by epriestley). · Explain WhySep 7 2016, 1:25 AM

This revision was automatically updated to reflect the committed changes.

Sorry if this is the wrong place for this, but do note that this breaks libphutil (and therefore arcanist) on Windows, since Process Control support and pcntl_signal() are not available on Windows according to http://www.php.net/manual/en/intro.pcntl.php.

sp@sp-lt2 MINGW64 /c/libphutil (master)
$ arc
[2016-09-07 06:31:38] EXCEPTION: (Error) Call to undefined function pcntl_signal() at [<phutil>\src\future\exec\PhutilSignalRouter.php:16]
phutil(head=master, ref.master=e94f6e738e2d)
#0 PhutilSignalRouter::initialize() called at [<phutil>\scripts\__init_script__.php:85]
#1 __phutil_init_script__() called at [<phutil>\scripts\__init_script__.php:91]
#2 include_once(string) called at [C:\arcanist\scripts\__init_script__.php:45]
#3 require_once(string) called at [C:\arcanist\scripts\arcanist.php:6]

epriestley mentioned this in T11603: libphutil briefly made unconditional calls to pcntl_signal(), fataling on Windows.Sep 7 2016, 1:57 PM