Page MenuHomePhabricator

Install a SIGTERM handler in ssh-connect
ClosedPublic

Authored by epriestley on Jun 13 2016, 4:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 6:25 AM
Unknown Object (File)
Wed, Dec 11, 10:11 PM
Unknown Object (File)
Tue, Dec 3, 8:11 PM
Unknown Object (File)
Tue, Nov 26, 7:38 AM
Unknown Object (File)
Tue, Nov 26, 7:38 AM
Unknown Object (File)
Tue, Nov 26, 7:18 AM
Unknown Object (File)
Nov 13 2024, 11:02 AM
Unknown Object (File)
Nov 8 2024, 8:19 PM
Subscribers
None

Details

Summary

Ref T10547. This has been around for a while but I was never able to reproduce it. I caught a repro case in the cluster recently and I think this is the right fix.

We tell Subversion to run ssh-connect instead of ssh so we can provide options and credentials, by using SVN_SSH in the environment. Subversion will sometimes kill the SSH tunnel subprocess aggressively with SIGTERM -- as of writing, you can search for SIGTERM in make_tunnel() here:

http://svn.apache.org/repos/asf/subversion/trunk/subversion/libsvn_ra_svn/client.c

By default, when a PHP process gets SIGTERM it just exits immediately, without running destructors or shutdown functions. Since destructors/shutdown functions don't run, TempFile doesn't get a chance to remove the file.

I don't have a clear picture of when Subversion sends SIGTERM to the child process. I can't really get this to trigger locally via svn, although I was able to get it to trigger explicitly. So I'm only about 95% sure this fixes it, but it seems likely.

Test Plan

Locally, I couldn't get this to reproduce "normally" even knowing the cause (maybe Subversion doesn't do the SIGTERM stuff on OSX?) but I was able to get it to reproduce reliabily by adding posix_kill(getmypid(), SIGTERM); to the body of the script.

With that added, running the script with PHABRICATOR_CREDENTIAL=PHID-CDTL-... in the environment reliably left straggler temporary files.

Adding declare() and a signal handler fixed this: the script now runs the TempFile destructor and longer leaves the stragglers around.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Install a SIGTERM handler in ssh-connect.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.Jun 13 2016, 4:41 PM
This revision was automatically updated to reflect the committed changes.