From 2d341e0cccae97fb40101bebc3eb97e5cf1b6eb0 Mon Sep 17 00:00:00 2001 From: Sergey Simonchik Date: Sat, 1 Feb 2025 10:07:47 +0100 Subject: [PATCH] [terminal] IJPL-176473 fix race when cancelling content updates might leave the last output pending/unapplied The problem is that `TerminalOutputChangesTracker` state is already updated, so `collectChangedOutputOrNull` sees the updated state, and therefore the pending/unapplied output is lost. `BlockTerminalCommandExecutionTest.commands are executed in order` is fixed now. Previously, it was flacky and the failure could be reproduced when running with "Repeat: Until failure". GitOrigin-RevId: 2f2431f930e15d5ef54ef62f1b9b13b90df72ae9 --- .../block/output/TerminalOutputChangesTracker.kt | 12 +++++++++++- .../output/TerminalOutputContentUpdatesScheduler.kt | 8 ++++++-- .../block/output/TerminalOutputController.kt | 13 +++++++------ .../block/BlockTerminalCommandExecutionTest.kt | 2 +- 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputChangesTracker.kt b/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputChangesTracker.kt index adf003b452e7..48c44c61e7a4 100644 --- a/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputChangesTracker.kt +++ b/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputChangesTracker.kt @@ -63,6 +63,10 @@ internal class TerminalOutputChangesTracker( private val changeListeners: MutableList<() -> Unit> = CopyOnWriteArrayList() + @Volatile + var pendingOutput: PartialCommandOutput? = null + private set + init { val listener = object : TextBufferChangesListener { override fun linesChanged(fromIndex: Int) = textBuffer.withLock { @@ -175,7 +179,9 @@ internal class TerminalOutputChangesTracker( isAnyLineChanged = false isChangesDiscarded = false - return PartialCommandOutput(output.text, output.styleRanges, logicalLineIndex, textBuffer.width, anyDiscarded) + return PartialCommandOutput(output.text, output.styleRanges, logicalLineIndex, textBuffer.width, anyDiscarded).also { + pendingOutput = it + } } /** @@ -190,4 +196,8 @@ internal class TerminalOutputChangesTracker( } return count } + + internal fun onOutputApplied() { + pendingOutput = null + } } diff --git a/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputContentUpdatesScheduler.kt b/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputContentUpdatesScheduler.kt index bfe78bc7f4de..92cf5593ee5b 100644 --- a/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputContentUpdatesScheduler.kt +++ b/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputContentUpdatesScheduler.kt @@ -56,6 +56,7 @@ internal class TerminalOutputContentUpdatesScheduler( val partialChange = tracker.collectChangedOutputOrWait() scheduleChangeApplying(partialChange).join() + tracker.onOutputApplied() } } @@ -73,13 +74,16 @@ internal class TerminalOutputContentUpdatesScheduler( } } - fun finishUpdating(): PartialCommandOutput? = textBuffer.withLock { + fun finishUpdating(): List = textBuffer.withLock { val tracker = changesTracker ?: error("Finish updating called before start updating") changesTracker = null updatingJob?.cancel() finished = true - tracker.collectChangedOutputOrNull() + // Not-null `tracker.pendingOutput` means that it was either not applied due to + // cancellation or is being applied right now on EDT. + // If the latter, it won't hurt to apply it twice. + return listOfNotNull(tracker.pendingOutput, tracker.collectChangedOutputOrNull()) } private val metricTextInBufferToTextVisible = ActionCoordinator( diff --git a/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputController.kt b/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputController.kt index 7d4b645f2f77..8ae3d3e5ce0b 100644 --- a/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputController.kt +++ b/plugins/terminal/src/org/jetbrains/plugins/terminal/block/output/TerminalOutputController.kt @@ -145,7 +145,7 @@ internal class TerminalOutputController( private fun scheduleLastOutputUpdate() { val contentUpdatesScheduler = runningCommandInteractivity?.contentUpdatesScheduler - val lastOutput: PartialCommandOutput? = if (contentUpdatesScheduler?.finished == false) { + val lastOutput: List = if (contentUpdatesScheduler?.finished == false) { contentUpdatesScheduler.finishUpdating() } else { @@ -155,18 +155,19 @@ internal class TerminalOutputController( val (output, terminalWidth) = session.model.withContentLock { ShellCommandOutputScraperImpl.scrapeOutput(session) to session.model.width } - PartialCommandOutput( + listOf(PartialCommandOutput( output.text, output.styleRanges, logicalLineIndex = 0, terminalWidth, isChangesDiscarded = false, - ) + )) } - - if (lastOutput != null) { + if (lastOutput.isNotEmpty()) { invokeLater(editor.getDisposed(), ModalityState.any()) { - updateCommandOutput(lastOutput) + for (output in lastOutput) { + updateCommandOutput(output) + } } } } diff --git a/plugins/terminal/tests/org/jetbrains/plugins/terminal/block/BlockTerminalCommandExecutionTest.kt b/plugins/terminal/tests/org/jetbrains/plugins/terminal/block/BlockTerminalCommandExecutionTest.kt index 87180af31eab..bc03b5c1c179 100644 --- a/plugins/terminal/tests/org/jetbrains/plugins/terminal/block/BlockTerminalCommandExecutionTest.kt +++ b/plugins/terminal/tests/org/jetbrains/plugins/terminal/block/BlockTerminalCommandExecutionTest.kt @@ -64,7 +64,7 @@ internal class BlockTerminalCommandExecutionTest(private val shellPath: Path) { expected.forEach { session.commandExecutionManager.sendCommandToExecute(it.command) } - awaitBlocksFinalized(view.outputView.controller.outputModel, count) + awaitBlocksFinalized(view.outputView.controller.outputModel, count, 60.seconds) val actual = view.outputView.controller.outputModel.collectCommandResults() Assert.assertEquals(expected, actual) }