Fix bg cmds #628
Fix bg cmds #628
Conversation
The kill applet in the current busybox executable does not support the `--` syntax therefore remove from the template command.
The signal parameter was being ignored and instead always sending the KILL signal instead.
devlib/connection.py
Outdated
| # Otherwise wait for the full timeout and kill it | ||
| if self.poll() is None: | ||
| # Otherwise wait for the full timeout and if still alive kill it | ||
| if self.poll() is None and self.redirect_thread.is_alive(): |
There was a problem hiding this comment.
@douglas-raillard-arm I'm not sure if this is the best way to detect whether the the command is still alive (or if it even makes sense as it is).
Do you have any thoughts on a better way to handle this?
There was a problem hiding this comment.
self.poll() really should be enough. If the command is alive, it's supposed to return None. When it's dead, it returns the exit code. If that's not the case it's poll() that needs fixing, not its call sites.
The API is modeled after Popen.poll: https://docs.python.org/3/library/subprocess.html#subprocess.Popen.poll
There was a problem hiding this comment.
Ok cool thanks, during my testing poll() was always returning None regardless of the command status so will take a closer look at what is going on there.
There was a problem hiding this comment.
Which connection class where you using for your tests ?
There was a problem hiding this comment.
This was using an SSH connection.
There was a problem hiding this comment.
Ok I'll also try to have a look tomorrow then
There was a problem hiding this comment.
Thank you for the offer, however on further investigation please ignore me.
After resetting my environment and board I can no long reproduce the original issue so I think I must have had something in a strange state and things are now working as expected.
I am however tempted to leave the additional poll in place if that still makes sense.
There was a problem hiding this comment.
The issue is real, there was a missing return in wait() following #626 . I've been doing mostly Rust recently ...
Here is the fix, feel free to just integrate it in your PR if you want to avoid merge conflicts: #629
I am however tempted to leave the additional poll in place if that still makes sense.
The exact same condition is tested in ParamikoBackgroundCommand._poll so it won't add anything:
def _poll(self):
if self.redirect_thread.is_alive():
return None
elif self.chan.exit_status_ready():
return self.wait()
else:
return None
There was a problem hiding this comment.
The issue is real, there was a missing return
Oh great, thank you for finding that, I was a rather confused at what was going on..
The exact same condition is tested in ParamikoBackgroundCommand._poll so it won't add anything
Good point thanks, I'll drop this change and pull your commit in instead.
Lack of return statement in wait() was making it return None instead of the exit code. Add appropriate return statement in wait() and other function to ensure return value is not lost.
2 participants
Fix a number of issues discovered during testing of #618.