Skip to content
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

Adding kill built-in command in order to support job control. #52

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brk0v
Copy link

@brk0v brk0v commented Jun 29, 2022

Internal kill replaces jobs ids with the corresponding process
group ids and executes kill binary.

For example:

kill %1       -> kill -- -123
kill %1 %3    -> kill -- -123 -154
kill -9 %1    -> kill -9 -123
kill -TERM %1 -> kill -TERM -123

Please, let me know if you want such or any changes.
If yes, I'm ready to work on it and update this PR.
If not, thank you for your work, I was learning how shells work and found your project. It helped me with understanding some internals.

Internal kill replaces jobs ids with the corresponding process
group ids and executes kill binary.

For example:

kill %1       -> kill -- -123
kill %1 %3    -> kill -- -123 -154
kill -9 %1    -> kill -9 -123
kill -TERM %1 -> kill -TERM -123
@michaelmacinnis
Copy link
Owner

Looks great. Apologies for you having to wade through the code in job_unix which is still quite a mess. I should have some time this weekend to review this more thoroughly.

@@ -233,6 +237,50 @@ func jobs(t *task.T) task.Op {
return t.Return(status.Int(0))
}

func kill(t *task.T) task.Op {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial thought was to make this a bit easier to write by exporting a few actions from internal/engine/task/action.go:

var (
    ExecMethod = Action(execMethod)
    External   = Action(external)
    Resume     = Action(resume)
)

so that kill could look something like this:

func kill(t *task.T) task.Op {
    if t.Code() == pair.Null {
        panic("expected at least 1 argument, passed 0")
    }

    args := []cell.I{}
    groups := []cell.I{}

    for a := t.Code(); a != pair.Null; a = pair.Cdr(a) {
        c := pair.Car(a)

        s := common.String(c)
        if strings.HasPrefix(s, "%") {
            n, err := strconv.Atoi(s[1:])
            if err != nil {
                panic("job does not exist")
            }

            group := job.GetGroup(n)
            if group == 0 {
                panic("job does not exist")
            }

            groups = append(groups, str.New("-" + strconv.Itoa(group)))
        } else {
            args = append(args, c)
        }
    }
    
    t.ReplaceOp(task.Resume)
    t.PushOp(task.External)

    t.PushResult(nil)
    t.PushResult(str.New("kill"))

    for _, c := range args {
        t.PushResult(c)
    }

    if len(groups) > 0 {
        t.PushResult(str.New("--"))

        for _, c := range groups {
            t.PushResult(c)
        }
    }

    return t.PushOp(task.ExecMethod)
}

and then update bg and fg to also support the percent syntax (by just stripping the % from arguments passed to those commands).

But the bigger problem is that jobs don't work the same way in oh as they do in other shells. In oh, the list of jobs is a list of currently suspended tasks that can be resumed. Oh's bg command removes a command from the list of jobs and returns a reference to that command that can be passed to wait. The bg command behaves like the spawn command. (Note that & in oh is rewritten as spawn).

To see what I mean try executing:

define t: spawn {
    sleep 60; echo done
}

jobs
# Should output nothing.

wait $t

The effect is the same as doing:

sleep 60; echo done

# Press Ctrl-Z to suspend the sleep.

define t: bg 1

jobs
# Should output nothing.

wait $t

Oh also avoids launching processes so not every job is part of a new process group (as they are part of the same process group as the shell - indicated with a group of 0) and as such the only signals that really make sense for those jobs are ones that terminate the job.

Because every job in oh's job list is suspended , we have to do a kill %n and then a fg n or bg n to continue the job which will then act on the signal sent by kill.

If we want a convenience wrapper to terminate a job we have suspended we should add a command called terminate (or something similar) that takes the same kind of arguments as wait so that we can do:

terminate (bg n)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants