dive icon indicating copy to clipboard operation
dive copied to clipboard

[FEATURE] Add way to cancel job in desktop version

Open mattdawkins opened this issue 3 years ago • 5 comments

Hit an error with a pipeline running forever

mattdawkins avatar Sep 30 '21 13:09 mattdawkins

Gathering some knowledge for what is required for this task. https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options

https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal

We don't save the job object, we do save the PID but that is only valid for the life of the process. After the process is complete we need to make sure we aren't killing a different PID.

Checking the code there is a processManager.ts which seems to have the functionality to close child processes started by the system. Within there is a children which is a list of all of the child processes. I could add a function here which does a search through existing jobs that haven't ended for the PID and then closes a specific one.

Once that part is setup I just need to add an interface to the jobs panel where if it is running there is a cancel button which will send a signal to the process to exit. Need to check on the subprocess shell issue that occurs with linux shell process commands specifically this quote under the subprocess_kill_signal: "On Linux, child processes of child processes will not be terminated when attempting to kill their parent. This is likely to happen when running a new process in a shell or with the use of the shell option of ChildProcess:"

BryonLewis avatar Oct 06 '21 17:10 BryonLewis

I don't know how similar Node's runtime is to UNIX proper, but the usual thing to do would be to catch SIGCHLD (which a child process emits to the parent when it terminates) and use that to update a list of active child processes. You could just check that table when attempting to kill a child process by it's PID. Your approach of checking the list of the processes is probably fine too. And one last idea: it's probably fine to just attempt to kill by PID anyway: for one thing, you're not likely to see a new process with the same PID show up for a very, very long time, and also even if that did happen, you simply wouldn't be able to kill it (unless you're running as root I suppose).

Anyway it's been a while since I worked with these concepts directly, so I may just be blowing smoke. Proceed as you see fit.

waxlamp avatar Oct 06 '21 17:10 waxlamp

Yeah there is infrastructure already for killing by PID and I can use that for the process so that is what I'm using.

To represent the state of a Job we were only using the exitCode provided by the process, null for currently running jobs 0 for success and non-0 for failures/errors. Problem is when killing/cancelling a job it just returns signal that was used instead of updating the exit code so I'll have to switch all reporting of status to be a combination of exitCode/Signal instead of just exitCode.

Node 'exit' event Quote I'm referencing

The 'exit' event is emitted after the child process ends. If the process exited, code is the final exit code of the process, otherwise null. If the process terminated due to receipt of a signal, signal is the string name of the signal, otherwise null. One of the two will always be non-null.

BryonLewis avatar Oct 11 '21 13:10 BryonLewis

This is being placed on hold until we can guarantee that there are no abandoned child processes when sending signals to the launching process.

BryonLewis avatar Oct 18 '21 15:10 BryonLewis

According to this stack overflow answer, this should be possible to do in a Gordian knot kind of way.

waxlamp avatar Jan 12 '22 21:01 waxlamp