Page MenuHomePhabricator

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

Authored by wotte on Dec 19 2013, 12:36 AM.
Tags
None
Referenced Files
F14013897: D7797.diff
Sat, Nov 2, 6:46 PM
F14010127: D7797.id17642.diff
Thu, Oct 31, 5:14 AM
F14009166: D7797.id.diff
Wed, Oct 30, 11:28 AM
F14009165: D7797.id17638.diff
Wed, Oct 30, 11:28 AM
F14009142: D7797.id17641.diff
Wed, Oct 30, 10:42 AM
F14009141: D7797.id17642.diff
Wed, Oct 30, 10:42 AM
F14009140: D7797.id17640.diff
Wed, Oct 30, 10:42 AM
F14001399: D7797.id17642.diff
Fri, Oct 25, 7:29 AM

Details

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Hmm, let's:

  1. Put this in libphutil/scripts/__init_script.php instead.
  2. 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.
  3. 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.
  4. Just install ABRT, and only log on it? You can always kill the process afterward with TERM or KILL, afterward, right?
  5. Use sys_get_temp_dir() instead of hardcoding /tmp.
  6. Use getmypid() (which is widely available) over posix_getpid() (which depends on the posix extension).
  7. Use Filesystem::writeFile() over fopen() + fwrite().
  8. 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.

wotte updated this revision to Unknown Object (????).Dec 19 2013, 1:28 AM

Applied requestd changes. Same testing as before.

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

wotte updated this revision to Unknown Object (????).Dec 19 2013, 1:31 AM

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).