Repository navigation
Conversation
mitchellh
left a comment
mitchellh
left a comment
There was a problem hiding this comment.
I'd prefer to note change blocking queue. I haven't looked at the code recently but I thought originally that during teardown we always drain queues. We should do that. Or spin off a thread to do that until its empty or something.
c777bd1 to
ce2d7e2
Compare
|
Switched to an approach that does not change BlockingQueue. |
ce2d7e2 to
6c9a9fb
Compare
6c9a9fb to
2aa4551
Compare
2aa4551 to
8eb9676
Compare
|
I tested this with #9760, which makes it much more likely to produce the thread hangs this change tackles. I found one remaining deadlock case: main in |
8eb9676 to
663e1ac
Compare
Thanks for finding that. I fixed that and I also found another similar deadlock that I fixed as well. |
|
I think that I have a way to simplify this… |
663e1ac to
cceab5a
Compare
jparise
left a comment
jparise
left a comment
There was a problem hiding this comment.
This looks good. I can no longer reproduce any thread hangs with #9760.
One question from reading the code: the termio drain starts after subprocess.stop(). If the read thread is already blocked on a full IO mailbox at that point, could killPid wait on a child that can't exit, before the drain starts? I didn't reproduce this, but starting the drain before stop() might rule it out.
cceab5a to
c75b793
Compare
Yeah, I think you're right. I moved the start of the drain before stopping the thread. |
Closing a surface whose child was still writing could hang Ghosthub until it was force quit (#269). The main thread frees the surface and waits for its threads to stop. The pty reader can be stuck sending messages to the app mailbox, which only the main thread empties. It can also be stuck sending to the renderer or io mailbox after those threads stop. While the reader is stuck, nothing reads the pty. The child's login wrapper cannot finish exiting until its output drains, so the io thread waits for it forever. Upstream Ghostty has an open fix, ghostty-org/ghostty#14444, that keeps these mailboxes drained while the threads are joined. Carry that diff unchanged and apply it with git apply before Ghosthub's own source patches. Then the carried copy can be deleted when the pinned revision includes the fix. The bootstrap version bump rebuilds cached libghostty artifacts with the fix. The trailing-whitespace hook now skips patch files, because removing the single-space blank context lines would corrupt the diff. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #269. User-facing benefit: closing a terminal no longer freezes Ghosthub. - **What was broken:** closing a surface whose program was still writing output, such as a Herdr attach, could hang the whole app until it was force quit. The main thread waits for the surface's threads to stop. The pty reader could be stuck sending to the app mailbox, which only the main thread empties, or to the renderer or io mailbox after those threads stopped. While the reader was stuck, the pty stopped draining. The child's `/usr/bin/login` wrapper could not finish exiting, so libghostty's kill loop waited for it forever. - **Fix:** carry the open upstream fix [ghostty-org/ghostty#14444](ghostty-org/ghostty#14444) for [ghostty-org/ghostty#14245](ghostty-org/ghostty#14245) (the same hang, reported by another embedder). While a surface joins its threads, it keeps those mailboxes drained. Messages for the closing surface are dropped, and messages for other surfaces are handled on the next app tick. - **How it is carried:** the upstream diff is vendored unchanged in `Vendor/ghostty-patches/surface-teardown-drain.patch`. The bootstrap applies it with `git apply` before Ghosthub's own source patches. Delete it once the pinned Ghostty revision includes the fix. The bootstrap version bump rebuilds cached libghostty artifacts. - **Regression test:** a smoke test runs a shell that rewrites the title in a tight loop, blocks the main thread right before the free so the app mailbox fills, and requires `shutdown()` to return. A watchdog aborts the run if the free hangs, because a frozen main thread cannot report a failure. - The `trailing-whitespace` hook now skips `.patch` files, because stripping single-space context lines would corrupt the diff. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <sup>generated by a clanker</sup> Co-authored-by: Marius van Niekerk <mariusvniekerk@users.noreply.github.com>
c75b793 to
efaa666
Compare
| /// Start draining. This must not be moved until `finish` returns. | ||
| pub fn start(self: *Self, io: std.Io) void { | ||
| self.io = io; | ||
| self.thread = std.Thread.spawn(.{}, run, .{self}) catch |err| thread: { |
There was a problem hiding this comment.
It might be helpful to give this thread a name (setName).
A thread that keeps a BlockingQueue drained, handing each value it takes off to a handler, while the queue's consumer can't: because it is joining threads that push into the queue, or because it has already been joined. A producer blocked on a full queue then still wakes up and the join returns.
Surface.deinit joins the search, renderer and IO threads from the app thread, which is the only thread that drains the app mailbox. A thread blocked pushing into a full mailbox never wakes, so the join never returns and the app thread is gone for good. The same happens one level down. Exec.threadExit joins the read thread after the IO thread's event loop has stopped, and the IO thread is the only consumer of its mailbox. And once the renderer thread is joined, the IO and read threads can block sending to the renderer. Each of these now keeps the mailbox drained with a QueueDrain until the join returns. App messages are set aside and handled on the next tick, apart from those for the surface being torn down. IO and renderer messages are thrown away, since nothing would ever have processed them. The IO drain starts before the subprocess is stopped. Stopping it waits for the child to exit, and a child that writes while handling SIGHUP can only exit if the read thread is still reading the pty. Fixes ghostty-org#14245
efaa666 to
3cc748e
Compare
Surface.deinit joins the search, renderer and IO threads from the app thread, which is the only thread that drains the app mailbox. A thread blocked pushing into a full mailbox never wakes, so the join never returns and the app thread is gone for good.
The same happens one level down. Exec.threadExit joins the read thread after the IO thread's event loop has stopped, and the IO thread is the only consumer of its mailbox. And once the renderer thread is joined, the IO and read threads can block sending to the renderer.
Each of these now keeps the mailbox drained with a QueueDrain until the join returns. App messages are set aside and handled on the next tick, apart from those for the surface being torn down. IO and renderer messages are thrown away, since nothing would ever have processed them.
Fixes #14245
AI disclosure: Claude Code was used in the development of this PR.