trbs / pid

Pidfile featuring stale detection and file-locking, can also be used as context-manager or decorator
https://pypi.python.org/pypi/pid/
Apache License 2.0
102 stars 26 forks source link

Default to lazy setup (and an additional check). #5

Closed nnathan closed 9 years ago

nnathan commented 9 years ago

In relation to https://github.com/trbs/pid/issues/4.

The following PR is a culmination of fixing the following issue:

with pid.PidFile(lazy=True):
    # (1) pid is locked at this point
    pidfile2 = pid.PidFile(lazy=True)
    with raising(pid.PidFileAlreadyRunningError):
        # (2) the pidfile2._setup() isn't called when performing check()
        pidfile2.check()

I made a test and the appropriate fix to support double lazy checks. I then tried the testsuite defaulting lazy=True and that also worked. Furthermore, I moved the logging to the _setup() phase, because we cannot make assumptions whether the internal logging stream handle would be available after daemonization. It isn't elegant but I beleive it is the correct approach when combining pid with a daemon manager.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.37%) to 88.89% when pulling 59a61f8dd39fa6c7418a5775f028f9f2a796a14f on nnathan:lazy into b233bfebdce258a835e9654de77bb78015006761 on trbs:lazy.

nnathan commented 9 years ago

You inferred the correct case, I like the third approach best. Let me fix up the code (after testing the TERM handler) and push then let's reevaluate the PR.

nnathan commented 9 years ago

trbs, thanks for the feedback. I've implemented the suggested changes:

(1) use the logger property to automatically setup a logging instance (and clean up close()), (2) remove the term_signal_handler you introduced, I didn't really see the obvious solution was to simply set the register_term_signal_handler=False. I was very confused by the TERM signal handler because I'm not quite familiar with atexit(), but now I understand the intentions when implementing this option.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.37%) to 88.89% when pulling 17692d1b05c91915ba633c11b8b471a6af3c5e5a on nnathan:lazy into b233bfebdce258a835e9654de77bb78015006761 on trbs:lazy.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.37%) to 88.89% when pulling 17692d1b05c91915ba633c11b8b471a6af3c5e5a on nnathan:lazy into b233bfebdce258a835e9654de77bb78015006761 on trbs:lazy.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.45%) to 88.82% when pulling 1cb6565c8e1ed681d26164462141ff2ee48d179b on nnathan:lazy into b233bfebdce258a835e9654de77bb78015006761 on trbs:lazy.

trbs commented 9 years ago

Nice !