Skip to content

Commit 520710f

Browse files
committed
make comment clearer
1 parent a2be937 commit 520710f

File tree

1 file changed

+21
-19
lines changed

1 file changed

+21
-19
lines changed

lib/termexec/termexec.go

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -49,27 +49,29 @@ func StartProcess(ctx context.Context, args StartProcessConfig) (*Process, error
4949
process := &Process{xp: xp, execCmd: execCmd}
5050

5151
go func() {
52-
// This is a hack to work around a concurrency issue in the xpty
53-
// library. The only way the xpty library allows the user to update
54-
// the terminal state is to call xp.ReadRune. Ideally, we'd just use it here.
55-
// However, we need to atomically update the terminal state and set p.lastScreenUpdate.
56-
// p.ReadScreen depends on it.
57-
// xp.ReadRune has a bug which makes it impossible to use xp.SetReadDeadline -
58-
// ReadRune panics if the deadline is set. So xp.ReadRune will block until the
59-
// underlying process produces new output.
60-
// So if we naively wrapped ReadRune and lastScreenUpdate in a mutex,
61-
// we'd have to wait for the underlying process to produce new output.
62-
// And that would block p.ReadScreen. That's no good.
52+
// HACK: Working around xpty concurrency limitations
6353
//
64-
// Internally, xp.ReadRune calls pp.ReadRune, which is what's doing the waiting,
65-
// and then xp.Term.WriteRune, which is what's updating the terminal state.
66-
// Below, we do the same things xp.ReadRune does, but we wrap only the terminal
67-
// state update in a mutex. As a result, p.ReadScreen is not blocked.
54+
// Problem:
55+
// 1. We need to track when the terminal screen was last updated (for ReadScreen)
56+
// 2. xpty only updates terminal state through xp.ReadRune()
57+
// 3. xp.ReadRune() has a bug - it panics when SetReadDeadline is used
58+
// 4. Without deadlines, ReadRune blocks until the process outputs data
6859
//
69-
// It depends on the implementation details of the xpty library, and is prone
70-
// to break if xpty is updated.
71-
// The proper way to fix it would be to fork xpty and make changes there, but
72-
// I don't want to maintain a fork now.
60+
// Why this matters:
61+
// If we wrapped ReadRune + lastScreenUpdate in a mutex, ReadScreen would
62+
// block waiting for new process output. This would make the terminal
63+
// appear frozen even when just reading the current state.
64+
//
65+
// Solution:
66+
// Instead of using xp.ReadRune(), we directly use its internal components:
67+
// - pp.ReadRune() - handles the blocking read from the process
68+
// - xp.Term.WriteRune() - updates the terminal state
69+
//
70+
// This lets us apply the mutex only around the terminal update and timestamp,
71+
// keeping reads non-blocking while maintaining thread safety.
72+
//
73+
// Warning: This depends on xpty internals and may break if xpty changes.
74+
// A proper fix would require forking xpty or getting upstream changes.
7375
pp := util.GetUnexportedField(xp, "pp").(*xpty.PassthroughPipe)
7476
for {
7577
r, _, err := pp.ReadRune()

0 commit comments

Comments
 (0)