phabricator_backtrace_[PID]
ClosedPublic
Actions

Authored by wotte on Dec 19 2013, 12:36 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rPHU5d7dc5cb30dd: Added a SIGABRT handler that will write a stack trace to…

Summary

Stack traces are useful for debugging things. This adds a
signal handler that registers SIGABRT and SIGHUP. In both cases, a
backtrace is written to /tmp/phabricator_backtrace_<PID>. If SIGABRT
is caught, we terminate the process with extreme prejudice.

Note, however, that any php script that wishes to actually use this
functionality must declare(ticks=1) (or some other value) in the main
script; i.e., setting this value in an included script doesn't appear
to impact the main script.

Test Plan

<?php
declare(ticks=1);
require_once ('__init_script__.php');
posix_kill(posix_getpid(), SIGABRT);
sleep (1000000);
?>

for both SIGABRT and SIGHUP, did the same with kill -ABRT and kill -HUP.
Observed that it worked as intended.

Diff Detail

Branch

sigabrt_handler

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

Related discussion from Phabricator:

https://secure.phabricator.com/chatlog/channel/6/?at=106101

Hmm, let's:

Put this in libphutil/scripts/__init_script.php instead.
Install at the end of initialization rather than the beginning, since if we're spinning on init it's going to be obvious (everything will be broken) but if this piece of code breaks for some reason it would be good to have normal error handling set up. That is, if an install has error handling turned off, we want to install this handler after we turn it back on so we can get a reasonable error message.
Test for function_exists('pcntl_signal') before installing handlers, and just continue without a warning if the function isn't available. Particularly, it's undesirable to make arbitrary scripts echo to stdout, and I don't think this warning is very valuable.
Just install ABRT, and only log on it? You can always kill the process afterward with TERM or KILL, afterward, right?
Use sys_get_temp_dir() instead of hardcoding /tmp.
Use getmypid() (which is widely available) over posix_getpid() (which depends on the posix extension).
Use Filesystem::writeFile() over fopen() + fwrite().
Prefer project conventions: f() over f (), new Exception() over new Exception ().

Seem reasonable?

Seems reasonable. Question, however: I'm not sure that Filesystem::writeFile() is available at that point - a bit of a chicken and egg problem if you will.

Also: the one reservation I would have with swallowing the abort is if some other part of php depends on abort() from libc to terminate the process.

Hmm, maybe use HUP instead then?

If you install at the end of initialization, Filesystem should always be available, I think.

Applied requestd changes. Same testing as before.

Ahh, right. I was thinking like a C compiler instead of a php interpreter.

Changed the name of the signal handler function.

I'm going to get rid of the echo and just fail silently, but this looks great otherwise. Thanks!

Closed by commit rPHU5d7dc5cb30dd (authored by @wotte, committed by @epriestley).

Revision Contents
Changeset List

Path

Size

scripts/

__init_script__.php

18 lines

Diff 17640

View Options

scripts/__init_script__.php

Added a SIGABRT handler that will write a stack trace to /tmp/phabricator_backtrace_[PID]ClosedPublicActions