fix(display): harden termWriter lifecycle against review findings on … · docker/compose@c48995e · GitHub
Skip to content

Commit c48995e

Browse files
committed
fix(display): harden termWriter lifecycle against review findings on #14119
Transposes the applicable findings from the review of the ttyWriter fix (#14119) to the rewritten renderer: - Done waits for the refresh goroutine to exit (outside the lock), so nothing can repaint after Done returns and output printed right after an operation cannot be garbled by a stray frame - a nested Start retires the previous cycle instead of leaking its refresh goroutine until the parent context is cancelled - build suspension no longer leaks across cycles: Start resets it and starts a fresh block below whatever buildkit wrote - drop the dead strings.ToLower in startDependencies: the unexported start only reads projectName when no Project is passed The remaining findings are structural to this rewrite: Done is a single critical section, and per-cycle context cancellation replaces the doneSignal handshake. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
1 parent 22e5ccd commit c48995e

3 files changed

Lines changed: 95 additions & 11 deletions

File tree

‎cmd/display/tty.go‎

Lines changed: 40 additions & 9 deletions

‎cmd/display/tty_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"unicode/utf8"
2727

2828
"github.com/mattn/go-runewidth"
29+
"go.uber.org/goleak"
2930
"gotest.tools/v3/assert"
3031

3132
"github.com/docker/compose/v5/pkg/api"
@@ -167,6 +168,59 @@ func TestTerm_DoneReturnsAfterContextCancel(t *testing.T) {
167168
}
168169
}
169170

171+
// Done is reachable through the public api.EventProcessor interface, so a
172+
// caller isn't guaranteed to have called Start first: it must be a no-op,
173+
// not a nil dereference.
174+
func TestTerm_DoneBeforeStartDoesNotPanic(t *testing.T) {
175+
w, _, _ := newTermWriter(80, 24)
176+
w.Done("op", false)
177+
}
178+
179+
// Once Done returns, nothing repaints anymore: the refresh goroutine must
180+
// not survive the bracket, or a stray frame could garble whatever the caller
181+
// prints next (a prompt, container logs). goleak needs no settling sleep
182+
// precisely because Done waits for the goroutine to exit.
183+
func TestTerm_NoRenderGoroutineSurvivesDone(t *testing.T) {
184+
w, _, _ := newTermWriter(80, 24)
185+
w.Start(t.Context(), "pull")
186+
w.On(api.Resource{ID: "Image foo", Text: "Pulling", Status: api.Working})
187+
w.Done("pull", true)
188+
goleak.VerifyNone(t)
189+
}
190+
191+
// A Start before the matching Done (nested brackets are a misuse, but Full
192+
// is public API) must retire the previous cycle instead of leaking its
193+
// refresh goroutine until the parent context is cancelled.
194+
func TestTerm_NestedStartRetiresPreviousCycle(t *testing.T) {
195+
w, _, _ := newTermWriter(80, 24)
196+
ctx := t.Context()
197+
w.Start(ctx, "outer")
198+
w.Start(ctx, "inner")
199+
w.Done("inner", false)
200+
w.Done("outer", false)
201+
goleak.VerifyNone(t)
202+
}
203+
204+
// Build suspension is scoped to a cycle: an operation ending mid-build (a
205+
// failed build aborts the bracket with suspended still set) must not leave
206+
// the next operation's progress permanently silent.
207+
func TestTerm_StartResetsBuildSuspension(t *testing.T) {
208+
w, buf, _ := newTermWriter(80, 24)
209+
w.Start(t.Context(), "build")
210+
w.On(api.Resource{ID: "Image app", Text: api.StatusBuilding, Status: api.Working})
211+
w.Done("build", false)
212+
213+
w.Start(t.Context(), "create")
214+
w.On(api.Resource{ID: "container app-1", Text: "Creating", Status: api.Working})
215+
w.mu.Lock()
216+
w.repaint()
217+
w.mu.Unlock()
218+
w.Done("create", true)
219+
220+
frame := stripAnsi(buf.String())
221+
assert.Assert(t, strings.Contains(frame, "container app-1"), "expected the new cycle to paint, got: %q", frame)
222+
}
223+
170224
// The spinner animation must be a function of time, not of how often the
171225
// layout runs.
172226
func TestTerm_SpinnerIsTimeBased(t *testing.T) {

‎pkg/compose/run.go‎

Lines changed: 1 addition & 2 deletions

0 commit comments

Comments
 (0)