-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kill all pids when killing pash via signal #454
Conversation
Signed-off-by: DIMITRIS KARNIKIS <[email protected]>
Fix broken links
Merge future to main
- fix broken link for evaluation scripts - make markdown formatting consistent Signed-off-by: Vladislav Doster <[email protected]>
Signed-off-by: Konstantinos Kallas <[email protected]>
Signed-off-by: DIMITRIS KARNIKIS <[email protected]>
Fix for python pkgs in pash setup
Signed-off-by: dkarnikis <dkarnikis>
Signed-off-by: dkarnikis <dkarnikis>
Install netcat-openbsd
Signed-off-by: dkarnikis <dkarnikis>
Signed-off-by: DIMITRIS KARNIKIS <[email protected]>
Signed-off-by: DIMITRIS KARNIKIS <[email protected]>
CI setup fix
Signed-off-by: DIMITRIS KARNIKIS <[email protected]>
Signed-off-by: DIMITRIS KARNIKIS <[email protected]>
Dockerfiles fix
Signed-off-by: Dimitris Karnikis <[email protected]>
The only issue here is that I do not know any way to silence the produced output by the killed processes:
All the processes are killed however :) |
pa.sh
Outdated
## In case we kill pash via signal, kill all the pending processes | ||
function kill_all() { | ||
# common prefix for all the pash processes | ||
pkill -f -9 tmp/pash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be processes that run commands like grep
and sort
right? I don't think this kills those. Also, could this kill the processes of another pash
instance?
I think that in order to kill all processes we need to do something inside this: https://github.com/binpash/pash/blob/main/runtime/wait_for_output_and_sigpipe_rest.sh in addition to whatever we catch here.
Also, whenever killing processes, we also need to wait for them.
Also, this should not go to main
but to future
since it is an experimental change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually kills everything!
In the current state of the script yeah, it will kill other pash instances. However, don't we assume that only a single instance of pash is running on machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually kills everything!
I don't see why tmp/pash
would match a grep
that is left running. Maybe I am missing something though
In the current state of the script yeah, it will kill other pash instances. However, don't we assume that only a single instance of pash is running on machine?
I don't think this is a valid assumption. We did a lot of modifications to allow multiple instances of pash to be run by the same user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not mistaken, all the pash commands operate on /tmp/pash_XXXXX/blah
, that's why the pkill -f -9 tmp/pash
works for us.
Is there even a point to optimize the script to only kill the children of the current pash instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not mistaken, all the pash commands operate on /tmp/pash_XXXXX/blah, that's why the pkill -f -9 tmp/pash works for us.
A grep in the dataflow might read and write to two completely arbitrary files, therefore I am not sure if /tmp/pash
would appear anywhere in its invocation.
Is there even a point to optimize the script to only kill the children of the current pash instance?
This is not an optimizations but rather correctness! If two instances of pash are running we don't want to kill both because one is killed.
I quite confident that the issue with the failing tests on the CI are already fixed on the fixes PR |
Closing this PR. Moved to #458 |
This should fix #452
Signed-off-by: Dimitris Karnikis [email protected]