twisted.internet.task.react should actually exit python, using appropriate exit codes

[itamar]

Exit codes tend to be more useful for command-line programs (the use case for react) than servers. As such, it seems like it'd be good if react actually exited with an exit code after the reactor is done.

• By default exit code should be 0.
• If an exception is raised/result Deferred is fired with errback, it should be 1.
• If a SystemExit exception is raised or passed as errback of result Deferred, exit with the code specified by the SystemExit.

This is an API change from current version, so we should decide if we're doing this before the next release since the API is still unreleased and therefore does not require backwards compat.

[therve]

(In [36479]) Branching to 'react-exit-6159'

[therve]

OK, this is ready for review. This was definitely harder than I expected, and the refactoring is certainly big. One questionable thing I changed is due to a problem I encountered when testing manually, illustrated by the new test_synchronousStop. I'm not sure why we support the user calling reactor.stop himself, though, as he doesn't call reactor.run.

[exarkun]

Thanks!

I think supporting the user calling reactor.stop themselves is important in order to make react generally useful, rather than something that will randomly break depending on which particular code you happen to be invoking (directly or indirectly - in the glorious future lacking a global reactor, this may be less of an issue, but for now it's too easy for any random code to stop the reactor on its own).

1. The news fragment should be a misc, since 3270.feature already describes react.
2. I'm surprised to see the code changed to depend on reactor.running. I think running is an implementation detail that we very grudgingly exposed, with fairly complicated and subtle semantics (for example, I don't think _FakeReactor is faithfully reproducing the transition to False, which should happen before the first application-added during shutdown hook runs, but in _FakeReactor it happens after the last during shutdown hook runs - which I think means the reactor.stop() support is actually broken against a real reactor) to satisfy people who were already using it or who find themselves in some particularly terrible situation - not something to actually use in our own code (at least not our code outside of the reactor implementation). I guess the documentation could be more clear on that point... Still, I wonder what was wrong with the previous approach, and whether that could be fixed instead of introducing this running usage.
3. It might be nice if all the unit tests made an assertion about the SystemExit's code (most do, but test_runsUntilSyncCallback, test_runsUntilSyncErrback, test_arguments, test_defaultReactor don't).
4. Can you file a ticket for adding unit tests for _FakeReactor?

[therve]

Thanks, everything fixed and #6200 opened. The change to running was probably due to some confusion about _FakeReactor behavior, and it was reverted.

[exarkun]

Thanks.

1. One case is not well handled with the latest version of the code - the case where something calls reactor.stop() and the Deferred returned by the main function does not quickly (ie, before reactor.run returns) fire. This is easy demonstrated with a main function that returns a Deferred that never fires:

from twisted.internet.task import react
from twisted.internet import defer

def main(reactor):
    return defer.Deferred()

react(main, [])

Run this and hit Ctrl-C. The result is an IndexError while trying to determine the proper exit code to use.

Once that case is fixed and buildbot is green, please merge.

[therve]

(In [36544]) Merge react-exit-6159

Author: therve Reviewer: exarkun Fixes: #6159

Change twisted.internet.task.react to exit the python process with non-0 exit code when the deferred returned failed.