twistd should wait for the application to start when daemonizing

[spiv]

Currently, twistd can start successfully (exit with code 0) even if the application fails when it daemonizes. It'd be good if it waited for the application to start, returning the appropriate exit code.

[spiv]

It would be nice if twistd had features like:
   -w --wait   twistd will wait until all services in the app configuration have
               started before exiting.  --nodaemon conflicts with this option.
   -k --kill <pidfile>  stop a twisted app specified by <pidfile>

--wait would be useful for scripts (e.g. functional tests) that want to spawn a
twisted app, wait for it to be listening and ready, and then proceed.  --kill
would be convenient for tearing down those tests, and probably in other places.

--wait could be implemented by opening a pipe to the child before forking, and
getting the child application to write to the pipe (or close it?) to signal when
it has started all its services.

--kill could be implemented on POSIX rather stupidly by doing:
    while True:
        try:
            os.kill(pid, 0)
        except OSError:
            break

--wait is really the one I want, though.

[exarkun]

Shouldn't --wait just be the default? I don't ever want to not know when the twistd command I ran didn't actually do what I wanted it to do.

[therve]

(In [22480]) Branching to 'twistd-wait-823'

[therve]

(In [22481]) Throw this hack somewhere: it waits for the start of a daemonized twistd process, and report its status.

Refs #823

[therve]

(In [23470]) Branching to 'twistd-wait-823-2'

[therve]

(In [23471]) Reimplement the wait slightly differently

Refs #823

[zooko]

 http://tahoe-lafs.org/trac/tahoe-lafs/ticket/71 is a ticket for the Tahoe-LAFS project which I think would be fixed by using the --wait option if twistd supports that option.

[zooko]

There is discussion in the Tahoe-LAFS project about whether to adapt therve's idea into Tahoe-LAFS source code base and/or to contribute such patches to this ticket:  http://tahoe-lafs.org/trac/tahoe-lafs/ticket/71#comment:24

[zooko]

jml suggested on IRC that this ticket might be ready for review. I looked at the patch and it is missing tests. The patch adds the functionality of having the parent wait until the child process has started and has signalled success or failure to the parent through a pipe. To test that added functionality, we might extend test_daemonize().

The tests could check whether the parent correctly exits with 0 when the child's startApplication() returns successfully, and whether the parent correctly exits with non-zero when the child's startApplication() raises an exception. I think that would be sufficient testing for this patch.

In order to do this the new test_daemonize() would have to not patch out the daemonization (as currently done in UnixApplicationRunnerSetupEnvironmentTests.setUp(). Instead it could probably patch out os._exit().

I think that would be sufficient testing for this patch.

[exarkun]

(In [29952]) Branching to 'twistd-wait-823-3'

[therve]

(In [37525]) Branching to 'twistd-wait-823-4'

[therve]

Let's do this!

[jerub]

Some small comments:

The soup nazi seinfeld sketch is no longer funny. Please update this message to be something like "Daemonization tests are only run on unix platforms when twisted.scripts._twistd_unix is available"

if _twistd_unix is None:
    DaemonizeTests.skip = "No daemonize for you"

The alignment in protocols/test/test_basic.py is wrong for these lines:

                          [b'produce', b'hello world', b'unproduce',
                            b'goodbye'])

Please fix.

I really don't like flags with names that start with 'no', and the not optionsnowait? parameter illustrates why. Can you think of a better name for this option so that we don't have to employ double negatives?

[jerub]

Okay, I did a poor review: the code I looked at was a bad diff.

Test run is available here.

[therve]

OK, back to review. Unfortunately I haven't found a better name than no-wait, still opened to suggestions though.

[glyph]

Replying to jerub:

Test run is available here.

FYI: "here" isn't a link.

[glyph]

Replying to therve:

OK, back to review. Unfortunately I haven't found a better name than no-wait, still opened to suggestions though.

--rush? --hurry? :-)

[glyph]

 Build results.

[tom.prince]

If I were to use this in an application, I'd ideally like

1. any logs generated while starting up.
2. an application hook to indicate when startup is complete. Since .startService isn't asynchronous, often an application won't be ready by the time it returns.

I've got some hackish code for buildbot that does it  here.

[radix]

Questions:

1. Can you explain why --no-wait is necessary? I'd rather not have it unless there's a really compelling reason.
2. Why limit the message to 100 bytes?

Code changes:

3. You removed the 'os' parameter from daemonize, but the docstring still documents it.
4. There are some conflicts with trunk

Discussion:

5. I'm not super into monkeypatch-the-import; it's pretty sensitive to changes. But I guess we already do it elsewhere.
6. the move of the daemonize method was hard to review.
7. I notice that this branch doesn't implement --kill, but it's clear to me that --kill should be done in a separate ticket/branch, so that's cool.

Everything else about the change seems pretty reasonable, but I'm a little worried that there may be weird edge cases that this would break for. Oh well!

To address tom's point:

1. I think logs would be pretty cool, but a notification that the user should look at the log file is pretty good for now.

[therve]

Tom: your second point is interesting, but really unrelated to anything happening in that ticket. It's already 9 years old, let's not add random feature requests.