flux-core
flux-core copied to clipboard
flux-framework
libsubprocess: stopped state change not handled correctly
I noticed in the following test in libsubprocess:
ok 16 - subprocess state == RUNNING after flux_local_exec
ok 17 - flux_subprocess_kill SIGSTOP
# state_change_stopped: state = Stopped
ok 18 - subprocess state == STOPPED in state change handler
# state_change_stopped: state = Running
# state_change_stopped: state = Exited
ok 19 - flux_reactor_run returned zero status
Which obviously looks weird, that the state change callback got the "Stopped" state before "Running".
I think it's because in the child watcher we have:
if (WIFSTOPPED (p->status)) {
if (p->ops.on_state_change)
(*p->ops.on_state_change) (p, FLUX_SUBPROCESS_STOPPED);
}
on_state_change() is called immediately rather than going through the state change prep/check functions, e.g. right below this:
if (WIFEXITED (p->status) || WIFSIGNALED (p->status)) {
/* remote/server code may have set FAILED on fatal errors.
*/
if (p->state == FLUX_SUBPROCESS_RUNNING) {
p->state = FLUX_SUBPROCESS_EXITED;
state_change_start (p);
}
So we need to redo things to go through the state change prep/check functions. I am not quite sure how at the moment and it may take some thought. See the state_change_next() function:
static flux_subprocess_state_t state_change_next (flux_subprocess_t *p)
{
assert (p->state != FLUX_SUBPROCESS_FAILED);
switch (p->state_reported) {
case FLUX_SUBPROCESS_INIT:
/* next state must be RUNNING */
return FLUX_SUBPROCESS_RUNNING;
case FLUX_SUBPROCESS_RUNNING:
/* next state is EXITED */
return FLUX_SUBPROCESS_EXITED;
case FLUX_SUBPROCESS_EXITED:
case FLUX_SUBPROCESS_FAILED:
case FLUX_SUBPROCESS_STOPPED:
break;
}
/* shouldn't be possible to reach here */
assert (0);
}
simply assumes the next state after RUNNING is EXITED, but it could be STOPPED instead. I suspect some type of "state change queue" of sorts needs to be implemented to get all the states transmitted in the correct order.
Note that I created issue #5082, not so much because #5082 is that important, but if this issue is corrected, it might be wise to just handle that at the same time as STOPPED can transition to RUNNING (or a new as of yet named state).
Note to Self: We should also document all of the legal state transitions that can occur too.
I think the meaning of RUNNING here is "process was started and we have a pid", so it's not necessarily exclusive of the process being also STOPPED.
Would it simplify the problem if we allowed the state callback to be called multiple times per state and reserved some bits in the state value for "attributes", then make STOPPED an attribute? So you might get the state callback with the state set to RUNNING | STOPPED?
Leaving this alone might be OK too?
The main reason we added STOPPED was to detect when the broker launched an interactive process non-interactively 95a906161c525c6e6c10a9d4bac04dafa7cd2f89. If this is still working for now, I like the idea of leaving this alone until we hit an actual problem.
Would it simplify the problem if we allowed the state callback to be called multiple times per state and reserved some bits in the state value for "attributes", then make STOPPED an attribute? So you might get the state callback with the state set to RUNNING | STOPPED?
Hmmm. it sounds to me like maybe you just want to know that SIGSTOP was received by the process. Would a on_signal_cb() (or similar) callback be more appropriate, and such a callback can be called many times?
I guess I view the job states as a "state machine" of sorts, so the user seeing "stopped" before "running" sort of didn't make much sense to me. I think that state changes should go through the state change prep/check functions too. I can't remember why I did it that way, but I suspect it might have been for racy conditions similar to this, like a user seeing "EXIT" before "RUNNING".
Leaving this alone might be OK too?
By all means not high priority, just something I noticed and can be a longer term TODO.
Hmmm. it sounds to me like maybe you just want to know that SIGSTOP was received by the process.
We want to know if the process was stopped by delivery of a signal, not just that a signal was received by the process. Since the state change callback can be used to receive a notification when the process exits similar to waitpid(2), it seems wise to use the same interface to determine when the process is stopped like waitpid(2), rather than adding a new interface. This keeps things simple for now.
Since we don't have a use case for WCONTINUED, I'd hesitate to add that now, but it should be trivial (except for the required testing...)
We want to know if the process was stopped by delivery of a signal, not just that a signal was received by the process. Since the state change callback can be used to receive a notification when the process exits similar to waitpid(2), it seems wise to use the same interface to determine when the process is stopped like waitpid(2), rather than adding a new interface. This keeps things simple for now.
Ok. Then perhaps using @garlick's "attribute" idea is the way to go longer term for this. That would allow the notification to go through the prep/check as well.