Add a way to control process respawn. #301
Conversation
| @@ -343,8 +346,12 @@ def reap_processes(self): | |||
| self.reap_process(pid) | |||
|
|
|||
| @util.debuglog | |||
| def manage_processes(self): | |||
| """ manage processes | |||
| def manage_processes(self, respawn=False): | |||
There was a problem hiding this comment.
you are changing the existing default behavior here, right ? Shouldn't we have True as the default value ?
There was a problem hiding this comment.
this arg was initially named force_respawn, but it didn't feel right, and we didn't find a better name...
self.respawn (on a watcher) is True by default, so the default behavior remains unchanged: if self.respawn, then (re)spawn processes if needed.
However, if self.respawn is False, then check if the arg is True, and in this case only, (re)spawn the processes.
If the watcher.respawn is False, then processes should only be (re)spawned when explicitly asked (on a start() for example), not on each call of manage_processes (which is called periodically).
So, I guess this boils down to: what should the name of this argument be, to make it clear? Is force_respawn better?
There was a problem hiding this comment.
Nope, we're not, maybe should we stay with the "force_respawn" name, which will make things more explicit.
We shouldn't need to have a look at the docstring nor the implementation to understand what this is doing. I propose that we change it back to force_respawn.
|
Updated with your remarks. |
|
@ametaireau I still see the respawn option as being True in some spots and False in others |
|
Following up an IRC discussion. I think we have a vocabulary & complexity problem here. I don't understand why we need two different options here (respawn & force_respawn) It looks over-complex at first sight. If we are creating "single-shot" watcher, they should be specific watchers with their own internal behavior, and the controller and arbiter should work with them the same way they do with the usual one. Maybe we need a level of inheritance to clear up and simplify this feature. But as it stands I am lost in the code and I should not be |
| @@ -344,8 +347,7 @@ def reap_processes(self): | |||
|
|
|||
| @util.debuglog | |||
| def manage_processes(self): | |||
| """ manage processes | |||
| """ | |||
| """ manage processes. """ | |||
There was a problem hiding this comment.
nitpick, if we want to go all the way to PEP8, we need to start with an uppercase and remove spaces:
"""Manage processes."""
|
o/wise LGTM |
The watchers now have a "respawn" option (True per default) which can be set to False. This allows to have processes that are run only once and not respawned automatically.
|
LGTM! |
|
merged. |
The watchers now have a "respawn" option (True per default) which can be set to False. This allows to have processes that are run only once and not respawned automatically.
supersede #162.