From c465c02f2979b8a7bb028268df14dfefa3cd89bf Mon Sep 17 00:00:00 2001 From: Fraser Waters Date: Fri, 15 Mar 2024 20:37:31 +0000 Subject: [PATCH] Revert "Decouple persist and display events (#15529)" (#15705) # Description This reverts commit ff7a7b4975356e92f843ba3d2270b6b6a9ac2329. Fixes https://github.com/pulumi/pulumi/issues/15668. --- ...newlines-being-written-during-updates.yaml | 4 + pkg/backend/display/jsonmessage.go | 52 ++++++------- pkg/backend/display/progress.go | 52 ++++--------- pkg/backend/display/tree.go | 68 +++++++---------- pkg/backend/display/tree_test.go | 75 ------------------- 5 files changed, 69 insertions(+), 182 deletions(-) create mode 100644 changelog/pending/20240315--cli-display--fix-superfluous-newlines-being-written-during-updates.yaml diff --git a/changelog/pending/20240315--cli-display--fix-superfluous-newlines-being-written-during-updates.yaml b/changelog/pending/20240315--cli-display--fix-superfluous-newlines-being-written-during-updates.yaml new file mode 100644 index 000000000000..cc1c53e45e22 --- /dev/null +++ b/changelog/pending/20240315--cli-display--fix-superfluous-newlines-being-written-during-updates.yaml @@ -0,0 +1,4 @@ +changes: +- type: fix + scope: cli/display + description: Fix superfluous newlines being written during updates diff --git a/pkg/backend/display/jsonmessage.go b/pkg/backend/display/jsonmessage.go index 3c1bed936759..ef76c897cf80 100644 --- a/pkg/backend/display/jsonmessage.go +++ b/pkg/backend/display/jsonmessage.go @@ -79,7 +79,6 @@ type messageRenderer struct { opts Options isInteractive bool - display *ProgressDisplay terminal terminal.Terminal terminalWidth int terminalHeight int @@ -140,10 +139,6 @@ func (r *messageRenderer) Close() error { return nil } -func (r *messageRenderer) initializeDisplay(display *ProgressDisplay) { - r.display = display -} - // Converts the colorization tags in a progress message and then actually writes the progress // message to the output stream. This should be the only place in this file where we actually // process colorization tags. @@ -179,20 +174,21 @@ func (r *messageRenderer) writeSimpleMessage(msg string) { r.colorizeAndWriteProgress(makeMessageProgress(msg)) } -func (r *messageRenderer) println(line string) { +func (r *messageRenderer) println(display *ProgressDisplay, line string) { r.writeSimpleMessage(line) } -func (r *messageRenderer) tick() { +func (r *messageRenderer) tick(display *ProgressDisplay) { if r.isInteractive { - r.render(false) + r.render(display, false) } else { // Update the spinner to let the user know that that work is still happening. r.nonInteractiveSpinner.Tick() } } -func (r *messageRenderer) renderRow(id string, colorizedColumns []string, maxColumnLengths []int, +func (r *messageRenderer) renderRow(display *ProgressDisplay, + id string, colorizedColumns []string, maxColumnLengths []int, ) { row := renderRow(colorizedColumns, maxColumnLengths) if r.isInteractive { @@ -216,50 +212,50 @@ func (r *messageRenderer) renderRow(id string, colorizedColumns []string, maxCol } } -func (r *messageRenderer) rowUpdated(row Row) { +func (r *messageRenderer) rowUpdated(display *ProgressDisplay, row Row) { if r.isInteractive { // if we're in a terminal, then refresh everything so that all our columns line up - r.render(false) + r.render(display, false) } else { // otherwise, just print out this single row. colorizedColumns := row.ColorizedColumns() - colorizedColumns[r.display.suffixColumn] += row.ColorizedSuffix() - r.renderRow("", colorizedColumns, nil) + colorizedColumns[display.suffixColumn] += row.ColorizedSuffix() + r.renderRow(display, "", colorizedColumns, nil) } } -func (r *messageRenderer) systemMessage(payload engine.StdoutEventPayload) { +func (r *messageRenderer) systemMessage(display *ProgressDisplay, payload engine.StdoutEventPayload) { if r.isInteractive { // if we're in a terminal, then refresh everything. The system events will come after // all the normal rows - r.render(false) + r.render(display, false) } else { // otherwise, in a non-terminal, just print out the actual event. - r.writeSimpleMessage(renderStdoutColorEvent(payload, r.display.opts)) + r.writeSimpleMessage(renderStdoutColorEvent(payload, display.opts)) } } -func (r *messageRenderer) done() { +func (r *messageRenderer) done(display *ProgressDisplay) { if r.isInteractive { - r.render(false) + r.render(display, false) } } -func (r *messageRenderer) render(done bool) { - if !r.isInteractive || r.display.headerRow == nil { +func (r *messageRenderer) render(display *ProgressDisplay, done bool) { + if !r.isInteractive || display.headerRow == nil { return } // make sure our stored dimension info is up to date r.updateTerminalDimensions() - rootNodes := r.display.generateTreeNodes() - rootNodes = r.display.filterOutUnnecessaryNodesAndSetDisplayTimes(rootNodes) + rootNodes := display.generateTreeNodes() + rootNodes = display.filterOutUnnecessaryNodesAndSetDisplayTimes(rootNodes) sortNodes(rootNodes) - r.display.addIndentations(rootNodes, true /*isRoot*/, "") + display.addIndentations(rootNodes, true /*isRoot*/, "") maxSuffixLength := 0 - for _, v := range r.display.suffixesArray { + for _, v := range display.suffixesArray { runeCount := utf8.RuneCountInString(v) if runeCount > maxSuffixLength { maxSuffixLength = runeCount @@ -268,18 +264,18 @@ func (r *messageRenderer) render(done bool) { var rows [][]string var maxColumnLengths []int - r.display.convertNodesToRows(rootNodes, maxSuffixLength, &rows, &maxColumnLengths) + display.convertNodesToRows(rootNodes, maxSuffixLength, &rows, &maxColumnLengths) removeInfoColumnIfUnneeded(rows) for i, row := range rows { - r.renderRow(strconv.Itoa(i), row, maxColumnLengths) + r.renderRow(display, strconv.Itoa(i), row, maxColumnLengths) } systemID := len(rows) printedHeader := false - for _, payload := range r.display.systemEventPayloads { + for _, payload := range display.systemEventPayloads { msg := payload.Color.Colorize(payload.Message) lines := splitIntoDisplayableLines(msg) @@ -307,7 +303,7 @@ func (r *messageRenderer) render(done bool) { } if done { - r.println("") + r.println(display, "") } } diff --git a/pkg/backend/display/progress.go b/pkg/backend/display/progress.go index ee19f071be20..7d9eeb0e626d 100644 --- a/pkg/backend/display/progress.go +++ b/pkg/backend/display/progress.go @@ -22,7 +22,6 @@ import ( "runtime" "sort" "strings" - "sync" "time" "unicode" @@ -66,19 +65,15 @@ type DiagInfo struct { type progressRenderer interface { io.Closer - initializeDisplay(display *ProgressDisplay) - tick() - rowUpdated(row Row) - systemMessage(payload engine.StdoutEventPayload) - done() - println(line string) + tick(display *ProgressDisplay) + rowUpdated(display *ProgressDisplay, row Row) + systemMessage(display *ProgressDisplay, payload engine.StdoutEventPayload) + done(display *ProgressDisplay) + println(display *ProgressDisplay, line string) } // ProgressDisplay organizes all the information needed for a dynamically updated "progress" view of an update. type ProgressDisplay struct { - // mutex is used to synchronize access to eventUrnToResourceRow, which is accessed by the treeRenderer - m sync.RWMutex - opts Options renderer progressRenderer @@ -139,6 +134,9 @@ type ProgressDisplay struct { // the list of suffixes to rotate through suffixesArray []string + // Maps used so we can generate short IDs for resource urns. + urnToID map[resource.URN]string + // Structure that tracks the time taken to perform an action on a resource. opStopwatch opStopwatch @@ -237,10 +235,10 @@ func ShowProgressEvents(op string, action apitype.UpdateKind, stack tokens.Stack eventUrnToResourceRow: make(map[resource.URN]ResourceRow), suffixColumn: int(statusColumn), suffixesArray: []string{"", ".", "..", "..."}, + urnToID: make(map[resource.URN]string), displayOrderCounter: 1, opStopwatch: newOpStopwatch(), } - renderer.initializeDisplay(display) ticker := time.NewTicker(1 * time.Second) if opts.deterministicOutput { @@ -255,7 +253,7 @@ func ShowProgressEvents(op string, action apitype.UpdateKind, stack tokens.Stack } func (display *ProgressDisplay) println(line string) { - display.renderer.println(line) + display.renderer.println(display, line) } type treeNode struct { @@ -312,11 +310,6 @@ func (display *ProgressDisplay) getOrCreateTreeNode( } func (display *ProgressDisplay) generateTreeNodes() []*treeNode { - // We take the reader lock here because this is called from the renderer and reads from - // the eventUrnToResourceRow map - display.m.RLock() - defer display.m.RUnlock() - result := []*treeNode{} result = append(result, &treeNode{ @@ -447,10 +440,6 @@ func removeInfoColumnIfUnneeded(rows [][]string) { // Specifically, this will update the status messages for any resources, and will also then // print out all final diagnostics. and finally will print out the summary. func (display *ProgressDisplay) processEndSteps() { - // Take the read lock here because we are reading from the eventUrnToResourceRow map - display.m.RLock() - defer display.m.RUnlock() - // Figure out the rows that are currently in progress. var inProgressRows []ResourceRow if !display.isTerminal { @@ -469,7 +458,7 @@ func (display *ProgressDisplay) processEndSteps() { // since the display was marked 'done'. if !display.isTerminal { for _, v := range inProgressRows { - display.renderer.rowUpdated(v) + display.renderer.rowUpdated(display, v) } } @@ -477,7 +466,7 @@ func (display *ProgressDisplay) processEndSteps() { // messages from a status message (since we're going to print them all) below. Note, this will // only do something in a terminal. This is what we want, because if we're not in a terminal we // don't really want to reprint any finished items we've already printed. - display.renderer.done() + display.renderer.done(display) // Render the policies section; this will print all policy packs that ran plus any specific // policies that led to violations or remediations. This comes before diagnostics since policy @@ -818,14 +807,10 @@ func (display *ProgressDisplay) processTick() { // often timeout a process if output is not seen in a while. display.currentTick++ - display.renderer.tick() + display.renderer.tick(display) } func (display *ProgressDisplay) getRowForURN(urn resource.URN, metadata *engine.StepEventMetadata) ResourceRow { - // Take the write lock here because this can write the eventUrnToResourceRow map - display.m.Lock() - defer display.m.Unlock() - // If there's already a row for this URN, return it. row, has := display.eventUrnToResourceRow[urn] if has { @@ -1008,26 +993,19 @@ func (display *ProgressDisplay) processNormalEvent(event engine.Event) { contract.Failf("Unhandled event type '%s'", event.Type) } - display.renderer.rowUpdated(row) + display.renderer.rowUpdated(display, row) } func (display *ProgressDisplay) handleSystemEvent(payload engine.StdoutEventPayload) { - // We need too take the writer lock here because ensureHeaderAndStackRows expects to be - // called under the write lock - display.m.Lock() - defer display.m.Unlock() - // Make sure we have a header to display display.ensureHeaderAndStackRows() display.systemEventPayloads = append(display.systemEventPayloads, payload) - display.renderer.systemMessage(payload) + display.renderer.systemMessage(display, payload) } func (display *ProgressDisplay) ensureHeaderAndStackRows() { - contract.Assertf(!display.m.TryLock(), "ProgressDisplay.ensureHeaderAndStackRows MUST be called "+ - "under the write lock") if display.headerRow == nil { // about to make our first status message. make sure we present the header line first. display.headerRow = &headerRowData{display: display} diff --git a/pkg/backend/display/tree.go b/pkg/backend/display/tree.go index 4c27a122b543..d706b5874fef 100644 --- a/pkg/backend/display/tree.go +++ b/pkg/backend/display/tree.go @@ -35,8 +35,7 @@ type treeRenderer struct { opts Options - display *ProgressDisplay - term terminal.Terminal + term terminal.Terminal permalink string @@ -85,24 +84,20 @@ func (r *treeRenderer) Close() error { return r.term.Close() } -func (r *treeRenderer) initializeDisplay(display *ProgressDisplay) { - r.display = display +func (r *treeRenderer) tick(display *ProgressDisplay) { + r.render(display) } -func (r *treeRenderer) tick() { - r.markDirty() +func (r *treeRenderer) rowUpdated(display *ProgressDisplay, _ Row) { + r.render(display) } -func (r *treeRenderer) rowUpdated(_ Row) { - r.markDirty() +func (r *treeRenderer) systemMessage(display *ProgressDisplay, _ engine.StdoutEventPayload) { + r.render(display) } -func (r *treeRenderer) systemMessage(_ engine.StdoutEventPayload) { - r.markDirty() -} - -func (r *treeRenderer) done() { - r.markDirty() +func (r *treeRenderer) done(display *ProgressDisplay) { + r.render(display) r.ticker.Stop() r.closed <- true @@ -123,7 +118,7 @@ func (r *treeRenderer) print(text string) { contract.IgnoreError(err) } -func (r *treeRenderer) println(text string) { +func (r *treeRenderer) println(display *ProgressDisplay, text string) { r.print(text) r.print("\n") } @@ -138,21 +133,22 @@ func (r *treeRenderer) overln(text string) { r.print("\n") } -func (r *treeRenderer) render() { - contract.Assertf(!r.m.TryLock(), "treeRenderer.render() MUST be called from within a locked context") +func (r *treeRenderer) render(display *ProgressDisplay) { + r.m.Lock() + defer r.m.Unlock() - if r.display.headerRow == nil { + if display.headerRow == nil { return } // Render the resource tree table into rows. - rootNodes := r.display.generateTreeNodes() - rootNodes = r.display.filterOutUnnecessaryNodesAndSetDisplayTimes(rootNodes) + rootNodes := display.generateTreeNodes() + rootNodes = display.filterOutUnnecessaryNodesAndSetDisplayTimes(rootNodes) sortNodes(rootNodes) - r.display.addIndentations(rootNodes, true /*isRoot*/, "") + display.addIndentations(rootNodes, true /*isRoot*/, "") maxSuffixLength := 0 - for _, v := range r.display.suffixesArray { + for _, v := range display.suffixesArray { runeCount := utf8.RuneCountInString(v) if runeCount > maxSuffixLength { maxSuffixLength = runeCount @@ -161,7 +157,7 @@ func (r *treeRenderer) render() { var treeTableRows [][]string var maxColumnLengths []int - r.display.convertNodesToRows(rootNodes, maxSuffixLength, &treeTableRows, &maxColumnLengths) + display.convertNodesToRows(rootNodes, maxSuffixLength, &treeTableRows, &maxColumnLengths) removeInfoColumnIfUnneeded(treeTableRows) r.treeTableRows = r.treeTableRows[:0] @@ -172,10 +168,15 @@ func (r *treeRenderer) render() { // Convert system events into lines. r.systemMessages = r.systemMessages[:0] - for _, payload := range r.display.systemEventPayloads { + for _, payload := range display.systemEventPayloads { msg := payload.Color.Colorize(payload.Message) r.systemMessages = append(r.systemMessages, splitIntoDisplayableLines(msg)...) } + + r.dirty = true + if r.opts.deterministicOutput { + r.frame(true, false) + } } func (r *treeRenderer) markDirty() { @@ -183,9 +184,6 @@ func (r *treeRenderer) markDirty() { defer r.m.Unlock() r.dirty = true - if r.opts.deterministicOutput { - r.frame(true, false) - } } // +--------------------------------------------+ @@ -207,9 +205,6 @@ func (r *treeRenderer) frame(locked, done bool) { } r.dirty = false - contract.Assertf(r.display != nil, "treeRender.initializeDisplay MUST be called before rendering") - r.render() - termWidth, termHeight, err := r.term.Size() contract.IgnoreError(err) @@ -265,15 +260,8 @@ func (r *treeRenderer) frame(locked, done bool) { treeTableHeight = termHeight - systemMessagesHeight - statusMessageHeight - 1 r.maxTreeTableOffset = len(treeTableRows) - treeTableHeight + 1 - if r.maxTreeTableOffset < 0 { - r.maxTreeTableOffset = 0 - } scrollable := r.maxTreeTableOffset != 0 - if r.treeTableOffset > r.maxTreeTableOffset { - r.treeTableOffset = r.maxTreeTableOffset - } - if autoscroll { r.treeTableOffset = r.maxTreeTableOffset } @@ -282,11 +270,7 @@ func (r *treeRenderer) frame(locked, done bool) { // Ensure that the treeTableHeight is at least 1 to avoid going out of bounds. treeTableHeight = 1 } - if r.treeTableOffset+treeTableHeight-1 < len(treeTableRows) { - treeTableRows = treeTableRows[r.treeTableOffset : r.treeTableOffset+treeTableHeight-1] - } else if r.treeTableOffset < len(treeTableRows) { - treeTableRows = treeTableRows[r.treeTableOffset:] - } + treeTableRows = treeTableRows[r.treeTableOffset : r.treeTableOffset+treeTableHeight-1] totalHeight = treeTableHeight + systemMessagesHeight + statusMessageHeight + 1 diff --git a/pkg/backend/display/tree_test.go b/pkg/backend/display/tree_test.go index c7e4de6f1b05..434c6b97b69c 100644 --- a/pkg/backend/display/tree_test.go +++ b/pkg/backend/display/tree_test.go @@ -20,11 +20,7 @@ import ( "strings" "testing" - "github.com/pulumi/pulumi/sdk/v3/go/common/resource" - "github.com/pulumi/pulumi/sdk/v3/go/common/tokens" - "github.com/pulumi/pulumi/pkg/v3/backend/display/internal/terminal" - "github.com/pulumi/pulumi/pkg/v3/engine" "github.com/pulumi/pulumi/sdk/v3/go/common/diag/colors" "github.com/stretchr/testify/assert" ) @@ -54,8 +50,6 @@ func TestTreeFrameSize(t *testing.T) { treeRenderer := newInteractiveRenderer(term, "this-is-a-fake-permalink", Options{ Color: colors.Always, }).(*treeRenderer) - display := &ProgressDisplay{} - treeRenderer.initializeDisplay(display) // Fill the renderer with too many rows of strings to fit in the terminal. for i := 0; i < 1000; i++ { @@ -81,8 +75,6 @@ func TestTreeKeyboardHandling(t *testing.T) { treeRenderer := newInteractiveRenderer(term, "this-is-a-fake-permalink", Options{ Color: colors.Always, }).(*treeRenderer) - display := &ProgressDisplay{} - treeRenderer.initializeDisplay(display) // Fill the renderer with too many rows of strings to fit in the terminal. for i := 0; i < 1000; i++ { @@ -136,70 +128,3 @@ func TestTreeKeyboardHandling(t *testing.T) { } } } - -func TestTreeRenderCallsFrameOnTick(t *testing.T) { - t.Parallel() - - var buf bytes.Buffer - term := terminal.NewMockTerminal(&buf, 80, 24, true) - treeRenderer := newInteractiveRenderer(term, "this-is-a-fake-permalink", Options{ - Color: colors.Always, - }).(*treeRenderer) - display := &ProgressDisplay{ - stack: tokens.MustParseStackName("stack"), - eventUrnToResourceRow: make(map[resource.URN]ResourceRow), - renderer: treeRenderer, - } - treeRenderer.initializeDisplay(display) - // Stop the ticker, this prevents the eventHandler from calling the frame method - // and rendering the tree. - treeRenderer.ticker.Stop() - - // Fill the renderer with too many rows of strings to fit in the terminal. - for i := 0; i < 1000; i++ { - // Fill the renderer with strings that are too long to fit in the terminal. - display.handleSystemEvent(engine.StdoutEventPayload{ - Message: strings.Repeat("a", 1000), - Color: colors.Always, - }) - } - - // Mark a row as updated, this used to invoke render, it should now - // only mark the renderer as dirty. This is only cleared when the - // frame function is called. the ticker is currently stopped, so - // this should never happen - treeRenderer.rowUpdated(&resourceRowData{}) - - func() { - treeRenderer.m.Lock() - defer treeRenderer.m.Unlock() - assert.Truef(t, treeRenderer.dirty, "Expecting the renderer to be dirty until we explicitly call frame") - // the treeRenderer has never rendered, so the systemMessages array - // should be empty at this point - assert.Emptyf(t, treeRenderer.systemMessages, - "Not expecting system messages to be populated until rendering happens.") - assert.Equalf(t, "", buf.String(), "Nothing should have been written to the terminal yet") - }() - - // This should trigger a render, and reset the dirty flag to false. - // This is normally called by the ticker event in treeRenderer.eventHandler, but for the test - // we have stopped the ticker. - treeRenderer.frame(false, false) - - // If dirty is true here, then there was no render operation - func() { - treeRenderer.m.Lock() - defer treeRenderer.m.Unlock() - assert.Falsef(t, treeRenderer.dirty, "Expecting the renderer to not be dirty after a frame is called") - - // An observable consequence of rendering is that the treeRenderer now has an array of system messages - assert.Equalf(t, 1000, len(treeRenderer.systemMessages), - "Expecting 1000 system messages to now be in the tree renderer") - }() - - // Check that at least one system messages was written to the mock terminal - terminalText := buf.String() - assert.Contains(t, terminalText, "pulumi:pulumi:Stack") - assert.Contains(t, terminalText, "System Messages") - assert.Contains(t, terminalText, strings.Repeat("a", 1000)) -}