diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/detached/server.zig | 73 | ||||
| -rw-r--r-- | src/fs_service.zig | 11 | ||||
| -rw-r--r-- | src/fuse.zig | 7 |
3 files changed, 68 insertions, 23 deletions
diff --git a/src/detached/server.zig b/src/detached/server.zig index d8373149..8e6b92d4 100644 --- a/src/detached/server.zig +++ b/src/detached/server.zig @@ -394,11 +394,22 @@ const Source = union(enum) { client: u8, pty: u8, inotify, - /// The acme filesystem's descriptor. Its arm does nothing: being in the set - /// is the whole point, because a readable `/dev/fuse` must end the sleep so - /// that `pollFrame` — which runs after `pull_wait_input` returns — reaches - /// the drain. Answering it here instead would re-enter the core from inside - /// its own `pump`. + /// The acme filesystem's descriptor. Its arm does one thing only: notice + /// that the connection has gone. Being in the set is otherwise the whole + /// point, because a readable `/dev/fuse` must END THE SLEEP so that + /// `pollFrame` — which runs after `pull_wait_input` returns — reaches the + /// drain. + /// + /// The drain is not done HERE, and the reason is not re-entrancy: both + /// `pull_wait_input` and `push_poll_frame` are called from inside + /// `Pardes.pump`, so either would re-enter. It is that `dispatch` is + /// mid-iteration over the `fds[0..n]`/`src[0..n]` SNAPSHOT `waitInput` + /// built, and a filesystem request reaches `core.perform`, which drains the + /// whole effect ring — including a `push_spawn`, whose `Session.spawn` + /// closes a pane's master and forks a new one. A later `.pty` entry in the + /// same pass would then apply the old descriptor's `revents` to a brand new + /// one, and the `fd < 0` guard cannot see it because the fd is valid, + /// merely different. Same hazard as the client slots, same reason. fuse, }; @@ -486,9 +497,18 @@ pub const Session = struct { pub fn deinit(s: *Session) void { // First, and before the pane shells: a script blocked on `event` is // holding a kernel request, and `Fs.deinit` answers everything still - // parked and aborts the connection before unmounting. Leaving it until - // after the harvest would leave that reader in uninterruptible sleep - // for as long as the harvest takes. + // parked and ABORTS THE CONNECTION before unmounting, so that reader + // wakes with ENODEV while its own shell is still alive to run its exit + // path. After `closePty` it would be woken by a hangup instead, with + // nothing left to exit into. + // + // Not, as this comment first claimed, because the harvest takes time: + // `harvest` is `waitpid(WNOHANG)` in a loop and blocks for nothing. The + // one step here that CAN take milliseconds is this one, because + // `Fs.deinit` forks `fusermount3` and waits for it untimed — which is + // also why the `.quit` owed to every frontend now queues behind a + // subprocess. Worth knowing; not worth reordering, because the shells + // matter more than the milliseconds. if (s.fs) |f| { f.deinit(); s.fs = null; @@ -1083,11 +1103,16 @@ pub const Session = struct { /// Collect every child that has exited. /// /// `waitpid(-1)` and not a pid list, because the only children this process - /// forks are pane shells (`host_io.forkShell`) — so "any exited child" and - /// "an exited pane shell" are the same set — and because the pids a list - /// would hold are exactly the ones it cannot help with: a respawn closes a - /// master, and the shell that gets the hangup exits some milliseconds later - /// with its slot already reused by a different shell. + /// LEAVES UNREAPED are pane shells (`host_io.forkShell`) — so "any exited + /// child" and "an exited pane shell" are the same set — and because the pids + /// a list would hold are exactly the ones it cannot help with: a respawn + /// closes a master, and the shell that gets the hangup exits some + /// milliseconds later with its slot already reused by a different shell. + /// Anything else this process forks — `fusermount3`, from `Fs.mount`, + /// `sweepStale` and `Fs.deinit` — is reaped by its own spawner with a + /// pid-specific blocking wait before control returns here, so the set this + /// sees is still only shells. A future worker that forks and does not wait + /// would break that, and this is the sentence it has to come back and edit. /// /// Nothing here waits, so a session whose shells are all running pays one /// syscall that returns 0. Called once per poll round and again wherever a @@ -1305,7 +1330,17 @@ pub const Session = struct { // `pollFrame` after `pull_wait_input` returns reaches the drain. The // desktop shells buy the same wake with a thread; one poll slot is // cheaper and cannot race the loop. - if (s.fs) |f| if (f.fd >= 0) { + // ...and NOT once it is dead. `fuse_dev_poll` answers `EPOLLERR` as soon + // as the connection is gone, POSIX reports `POLLERR` whatever the events + // mask asked for, and this arm cannot consume it — so an external + // `fusermount3 -u`, a sysfs abort, or systemd taking `/run/user/$UID` + // away at final logout (exactly when a detached session is supposed to + // keep running) would make `poll(2)` return instantly, forever, and burn + // a whole core for the life of the daemon. Measured at 100% of one CPU + // before this guard. fuse.zig's own poll thread has carried the + // equivalent check all along, which is why the desktop shells never + // showed it and this loop did. + if (s.fs) |f| if (f.fd >= 0 and !f.dead) { fds[n] = .{ .fd = f.fd, .events = poll_in, .revents = 0 }; src[n] = .fuse; n += 1; @@ -1413,9 +1448,13 @@ pub const Session = struct { } }, .inotify => if (pfd.revents & poll_in != 0) s.drainInotify(), - // Nothing. See `Source.fuse`: the wake IS the work, and the drain - // belongs to `pollFrame`, where re-entering the core is legal. - .fuse => {}, + // The only revents worth a word: a dead connection must leave the + // set, or the `POLLERR` it reports on every future poll spins the + // loop. `Fs.next` would set `dead` on its first failed read anyway; + // saying it here costs nothing and saves the one spinning round. + .fuse => if (pfd.revents & (poll_hup | poll_err | poll_nval) != 0) { + if (s.fs) |f| f.dead = true; + }, }; } diff --git a/src/fs_service.zig b/src/fs_service.zig index 7821e850..b440c2c2 100644 --- a/src/fs_service.zig +++ b/src/fs_service.zig @@ -43,10 +43,13 @@ const max_batch = 64; /// listener beside the mount a matter of writing one more implementation /// rather than of teaching this file about it. /// -/// It is deliberately NOT a Zig interface with `anytype`: the two callers -/// (tty.zig and gui.zig) store the transport in a struct field across frames, -/// so it has to be a value with a runtime type, which is a vtable — the same -/// shape and the same reasoning as `host.VTable`. +/// It is deliberately NOT a Zig interface with `anytype`: `drain` is ONE +/// function reached from four call sites (tty.zig, gui.zig twice, and the +/// daemon), and the second transport this seam exists for is chosen at RUN +/// time, so it has to be a value with a runtime type — which is a vtable, the +/// same shape and the same reasoning as `host.VTable`. Nothing holds a +/// `Transport` across a frame: every caller builds one inline from whatever it +/// has, which is why the thunks matter and the struct does not. /// /// The ORDER contract stays where it was, in `drain`, because it belongs to /// the caller rather than to any implementor: `retry()` to null first, then diff --git a/src/fuse.zig b/src/fuse.zig index 3a903e8b..3bd263bd 100644 --- a/src/fuse.zig +++ b/src/fuse.zig @@ -59,8 +59,11 @@ const libc = std.c; const linux = std.os.linux; const acmefs = @import("acmefs.zig"); /// Only for `Transport`, the three-function shape this mount presents to the -/// host loop. No cycle: `fs_service` names no type from here any more, which -/// is the point of the seam. +/// host loop. This IS a cycle — `fs_service` imports this file back for +/// `Fs.mount`, `sweepStale` and `exportPaneEnv`, and still names `*Fs` in three +/// of its own signatures — and Zig accepts it because imports are analysed +/// lazily. What the seam removed is `drain`'s dependency on the concrete type, +/// not the file's dependency on this one. Do not read it as more than that. const fs_service = @import("fs_service.zig"); /// Everything below the mount is Linux kernel ABI. Off Linux the module still |