Launchpad itself

Merge lp:~jameinel/launchpad/lp-service into lp:launchpad

  1. lp-service
  2. Merge into devel
Proposed by John A Meinel
Status: Merged
Merge reported by: John A Meinel
Merged at revision: not available
Proposed branch: lp:~jameinel/launchpad/lp-service
Merge into: lp:launchpad
Diff against target: 0 lines
To merge this branch: bzr merge lp:~jameinel/launchpad/lp-service
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Pending
Launchpad code reviewers Pending
Review via email: mp+42129@code.launchpad.net

This proposal supersedes a proposal from 2010-11-08.

Description of the change

It looks like I accidentally targetted this to lp:launchpad/db-devel in the past, when it was meant to be targetted to lp:launchpad/devel. So I'm resubmitting. It has already been landed in db-devel, so there shouldn't be a reason to not land it into devel.

This adds a --pid-file option to "bzr launchpad-forking-service". Which then causes the service to Daemonize itself, and write its pid to the given file.

This was brought out of the discussion of trying to get the service rolled out into qastaging.
see https://rt.admin.canonical.com/Ticket/Display.html?=42199

It would seem that 'start-stop-daemon' could be told to do the forking and pid-file management (--background and --make-pidfile), but it seems to state that "This is a last resort, and is only meant for programs that either make no sense forking on their own, or where it's not feasible to add the code for them to do this themselves."

The change seems reasonably localized, and hopefully reasonably tested. In general with IPC, I'm not always sure how to avoid both race conditions, hangs, and spurious failures. I chose 2.0s as a way to wait for the process to cleanup properly, but not block forever and hang the test suite.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

Note that I still need some reviewer hand-holding to submit this to ec2, etc.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote : Posted in a previous version of this proposal

As I said on IRC, I think you should wait until the daemon process is accepting requests before proceeding in _start_subprocess.

The 'out' variable in _daemonize is strangely located and named -- can you call it something clearer and assign to it closer to its use? I don't think we need to chdir to /, so please delete that comment. I don't think we really need to setsid either, but it does no harm I guess.

It's a bit of a shame you had to write (or copy/paste) so much code for the process handling, but oh well. There are good reasons this daemon isn't using twistd.

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

setsid() is recommended from the daemonize code I've read:
  http://www.itp.uzh.ch/~dpotter/howto/daemonize

os.chdir('/') is sort of recommended, but doesn't seem to be required.

I moved the '/dev/null' definition closer.

I think that addresses everything. If you could give it a once-over and then send it via ec2land, I would appreciate it.

Preview Diff

Empty