Repository navigation
Conversation
|
Rebased on the reworked #1381 — this PR is stacked on it, so its first three commits are that one. I applied your #1381 review here too, without waiting to be told twice:
One thing I did not do, and would rather ask than guess. These do not convert as cleanly. Of the 41 cases, most assert with So
|
ea0361e to
a7c9fc8
Compare
8ab0f51 to
0b39dd0
Compare
|
Rebased on current master. The interesting merge was with the new $! handling from master: bgProc now carries both the fake-PID/started-report fields from master and this PR's job-control fields, background statements get both the started-program report and the detached cancellable context, and the wait builtin uses master's lookupBgProc together with the interruptible await from this PR. |
|
Thanks - please split this again, as it seems to me like job control has little to do with some of the non-trivial builtins you are adding here, like compgen and history. Ideally I don't want to be reviewing more than a few hundred lines per PR. |
0b39dd0 to
1f4e745
Compare
0pcom
changed the title
|
Done — split three ways, each independent against master:
The one seam between them: |
|
Thanks for splitting it. Four things before I look closer:
That's the common form in scripts. Inverted, too: a bare Detaching from the caller's context changes behavior for every embedder, not just interactive ones. Running
The tests leak. On the unit-test question: split it. The deterministic cases ( |
1f4e745 to
a427eba
Compare
|
All four addressed. The context detachment is now its own PR — #1418, stacked on this branch — and this PR is builtins-only again.
A bare integer that matches no job is signalled as a real process where the platform can (unix; an error on windows/plan9/js): before this PR those specs reached the system's kill program, and
Detachment — moved to #1418, gated on
Tests — the deterministic cases moved into |
|
One real bug left, and this revision made it matter more: Smaller, none blocking:
Finally, |
|
Signal names now resolve through the platform: One correction to the diagnosis, because I went looking for the misclassification and it is not where either of us thought. On darwin — and on FreeBSD, OpenBSD, NetBSD and DragonFly — 17 through 22 are exactly the six job-control signals in a different order, so the old range was accidentally correct there. The platform it actually broke on is solaris, where 17 is USR2 and 19–22 are PWR, WINCH, URG and IO: five signals that should end a job, refused as "no job control". The I cannot test darwin or solaris here, only Linux. The new tests are written entirely in terms of names and mention no number, so they assert the same thing on every platform and would have failed against the old table — but they have only been run on Linux. The rest:
Two left undone, deliberately. Finished jobs still never leave the table: matching bash means reusing job numbers, which renumbers |
|
CI caught that my bash does report "no such job", but not for the reason I implemented. Its rule is about whether the job has been reaped, not whether it is still running — a finished job that nothing has reported still answers: Testing So this is not a separate smaller item: The signal-name work is unaffected — that is the earlier commit and it was green on macos/1.26. Reverting only the |
|
Green apart from one unrelated flake. On 76ff8b6: macos 1.26 and 1.27, ubuntu 1.26, windows 1.26 and alpine all pass; ubuntu 1.27 was cancelled by fail-fast rather than failing. The one failure is Worth noting what the green macos runs bought: the signal tests are written in terms of names and had only ever been run on Linux, so this is the first time the platform lookup was actually exercised on darwin, where USR1 is 30 rather than 10. It passes there on both Go versions. |
76ff8b6 to
da5d4bf
Compare
|
Two things on the current revision.
For The comment justifying the bare-PID path isn't right either:
On master they don't — I don't have a strong opinion about what the right behavior here is, but at the very least, the comment is wrong. Process substitutions consume job numbers.
|
da5d4bf to
4034221
Compare
|
Both fixed in
Job numbers. Numbering now skips process substitutions. I did not take it off So Two regression tests cover them. Against the unfixed code the first one kills the test binary outright, which is the bug. |
|
I'm not sure what to do with this feature, to be entirely honest. Every time I look at it, I uncover new bugs. We might need to carefully come up with a plan and design before looking at code. Or perhaps job control is not something that this library should aim to do, if it's not a good fit for Go or for the current v3 API.
I think we should only allow killing by PID - or killing PIDs which we have not started ourselves - behind an opt-in flag, like #171. But for now I probably would just leave that as a TODO and just forbid it entirely.
|
4034221 to
a114300
Compare
|
You asked for a plan before more code, which is fair — the PR described what it What uses thisAn interactive shell in a browser tab: websh, That also explains the shape. A job is a goroutine, so What I found, and fixed in
|
The job table never shrank. A finished job stayed in it, was listed again by every later jobs, and pushed the next job's number up; bash drops a job once jobs or wait has reported it, which is why a second jobs prints nothing and why the numbering starts again from one. For an interactive shell embedding this — a browser terminal, in our case — the table also grew for the life of the session. Jobs now carry their number rather than deriving it from their position in the table, and the table holds pointers so that dropping one leaves the rest alone. $! reads the most recent job directly, so it still answers after that job has been reaped. This brings master in line with the sh-job-control branch, which is what mvdan#1382 proposes, so the same code runs in both places. Three behaviours change with it: - A bare integer in a job spec is a PID and never a job number, as in bash. `kill 1` no longer means job 1. - Signal names are resolved through the platform instead of a table compiled into the shell, which was Linux's numbering everywhere: SIGUSR1 is 10 on Linux and 30 on darwin, where 10 is SIGBUS. - wait takes a job specification as well as a PID. Listings gain the trailing `&` on a running job and bash's column width, and whether a job is running is read once per row: reading it twice let a job finishing mid-listing be reported as Running and reaped in the same breath, losing it before anything said it had ended.
jobs, kill, disown, fg and bg. A job here is a goroutine running a subshell rather than a process, so the builtins are shaped by what that can honestly mean: jobs lists them, kill cancels a job's context (which is what any fatal signal would have amounted to), disown drops it from the table and out of a bare wait, fg waits for it since there is no terminal to hand it, and bg reports on a job that is already running. Nothing is ever stopped, so SIGSTOP and SIGCONT are rejected rather than faked. This is also what makes them work on js/wasm, where there are neither processes nor signals. Job specs follow bash: the % forms, plus whatever $! expanded to — the real PID when the background statement was exactly one external program, or the fake gN pid otherwise — so `kill $!` always works. A bare integer is a PID, never a job number, and a PID that belongs to no job is signalled as a process where the platform can, since scripts reached the system's kill program before this builtin shadowed it. Signal names resolve through the platform rather than a hardcoded table: signalName and signalNum in os_unix.go ask x/sys/unix, effectOf switches on the name, and kill -l walks the host's own numbering. kill -0 reports on the job table only. bash's rule is about whether the shell has reaped the process, which needs reaping this package does not do, so implementing bash's answer here would have been a guess dressed as a check. help gains a notes table for builtins that cannot mean quite the same thing without processes or a controlling terminal, and prints the note after the description.
The job table never shrank. A finished job stayed in it, was listed again by every later jobs, and pushed the next job's number up; bash drops a job once jobs or wait has reported it, which is why a second jobs prints nothing and why the numbering starts again from one. In a long-lived interactive shell the table also grew without bound. Jobs now carry their number rather than deriving it from their position, and the table holds pointers so that dropping one leaves the rest alone. $! reads the most recent job directly, so it still answers after that job has been reaped, as bash does. Two smaller fixes alongside: wait takes a job specification and not only a PID, and a listing gets the trailing `&` on a running job and bash's column width. Reading whether a job is running is now done once per row, since reading it twice let a job finishing mid-listing be reported as Running and reaped in the same breath.
A job ended by kill reported the subshell's own exit status, so `sleep 5 & p=$!; kill "$p"; wait "$p"` answered 1 where bash answers 143. The jobs listing already said Terminated for that job, so the two disagreed about the same job. wait and fg now go through one place which gives a job kill ended 128 plus the signal number, as bash does, and the status a job which finished on its own had.
|
All three of the things you raised are fixed, and the design question is answered below. Jobs no longer die with the statement that started them.
On whether this belongs here. websh is an interactive shell in a browser tab built on this package, with no processes anywhere, and it runs One thing worth saying plainly, because it bounds what this can ever promise. Three commits, each building and passing on its own. If you would rather the package did not carry this, say so and I will close this and #1418. |
Take the versions under review upstream (mvdan#1382, mvdan#1418, mvdan#1412, mvdan#1411), including wait and fg reporting 128 plus the signal. kill now signals only this shell's own jobs and StopJobs takes a context, as in the PRs. Keep what only the fork had: the umask applied to redirections, compgen -A job and -A signal, the history and logout help notes, and the tinygo SameFile shim.
The PowerShell resolver selects a private .NET runtime and its shared libraries. Keep those child controls in the fence environment snapshot so the same Bash# source runs in the scratch image. Sprint: mvdan#358 Story: mvdan#1382 Story-ID: ff9c0f723cbf
A job is a background shell running as a goroutine rather than a process, so the job control builtins are kept to what that can mean: * jobs lists the jobs in Bash's format, with -p and job specs. A finished job is reported once and then dropped from the listing. * kill cancels a job's context, with any of the signals whose numbers POSIX fixes: HUP, INT, QUIT, ABRT, KILL, ALRM, and TERM. A killed job exits with 128 plus the signal number, as in Bash. kill -0 checks whether a job is still running, and kill -l works. * disown, with -a or job specs, drops jobs from the listing and from a plain wait. * wait also accepts job specs. * fg and bg fail with "no job control", like a non-interactive Bash, as nothing is ever stopped. Job specs are %N, %%, %+, %-, %prefix, or what $! expanded to, be it a fake PID or the real PID of an external program. Process substitutions do not take up job numbers. Nested jobs keep running once the job which started them finishes, and jobs still end with the context given to Run. Reset leaves jobs running like a shell which exited, now documented. Known differences with Bash: * kill only signals this runner's own jobs. Other PIDs, including 0 and -1, are "no such job", as signalling the host process or arbitrary processes from a library is dangerous; see #171. * Only the signals above are supported, and all of them cancel the job the same way. A job ignoring its context keeps running, and a job running a program is ended via the ExecHandler, not by the signal. STOP, CONT, and the other signals are invalid. * Job numbers are never reused, where Bash restarts from the highest number still listed. * Killing a job also kills the jobs it started. * A finished job is reported as Terminated if its exit status is 128 plus a supported signal number, even if it exited on its own. * jobs supports no options other than -p, disown has no -h nor -r, and %?str is not supported. * Subshells such as pipeline elements see no jobs, and nothing announces finished jobs at a prompt. Thanks to Moses Narrow for contributing the original implementation in #1382, which this design is based on. For #1401.
|
For the sake of unblocking this work, I felt like it was best that I use this PR as the basis for a new implementation that only aims to support the basics in terms of what I think users and scripts actually use most. c2cf6f2 is now in master with this; it's ~300 lines as opposed to the ~1300 here. I've attributed you in the commit message as well. Please take a look and let me know what you think; if there are features you're missing from that implementation, let me know and I can take a look. It's probably best to discuss in an issue rather than send patches, especially for large stuff like this PR. I'll go through your PRs one by one, so it's probably best that we finish with this one before we look at the rest. |
|
Thanks for taking this on and for the credit. The smaller design reads well, and the builtins cover what scripts use. I ran it against our browser shell and filed the two gaps it hits as #1440. I am closing this PR since master supersedes it. |
2 participants
A job here is a goroutine running a subshell rather than a process, so the builtins are shaped by what that can honestly mean:
jobslists them,killcancels a job's context (which is what any fatal signal would have amounted to),disowndrops it from the table and out of a barewait,fgwaits for the job since there is no terminal to hand it, andbgreports on a job that is already running. Nothing is ever stopped, so SIGSTOP/SIGCONT are rejected rather than faked. This is also what makes them work on js/wasm, where there are neither processes nor signals.The second commit detaches background jobs from the statement's context, because a job outlives the command line that started it: an interactive shell cancels a line's context when the line is done, and in bash the interrupt that ends a foreground job leaves the background ones running. What ends a job is
kill, the shell exiting, or the newRunner.StopJobs.helpgains a small notes table for builtins that cannot mean quite the same thing without processes or a controlling terminal.Split out of the earlier version of this PR as requested: umask is #1411 and enable/compgen/history/logout are #1412, each independent against master.
Part of #1401.