diff --git a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt index 73a0c87cdb..e7d65d9a08 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt @@ -104,12 +104,15 @@ import kotlinx.coroutines.cancel import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.greenrobot.eventbus.EventBus import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode import org.slf4j.LoggerFactory import java.io.File +import java.util.concurrent.atomic.AtomicInteger import kotlin.coroutines.resume fun interface OnEditorLongPressListener { @@ -141,7 +144,17 @@ open class IDEEditor private var actionsMenu: EditorActionsMenu? = null private var _signatureHelpWindow: SignatureHelpWindow? = null private var _diagnosticWindow: DiagnosticWindow? = null - private var fileVersion = 0 + + /** + * [documentChangeMutex] only serialises change dispatches against each other; the resets in + * [release] and [dispatchDocumentOpenEvent] run outside it, so a reset can race an in-flight + * [dispatchDocumentChangeEvent]'s `incrementAndGet()` and stamp a low version right after a + * newly-opened file's counter is zeroed. This is tolerated: it is bounded (self-heals on the + * next edit) and distinct from the same-document backwards-version bug this ticket fixes, + * which `ActiveDocument.update` now guards regardless of how `fileVersion` got there. + */ + private val fileVersion = AtomicInteger(0) + private val documentChangeMutex = Mutex() internal var isModified = false // Length and content hash of the content the last time the file was loaded or saved. @@ -570,7 +583,7 @@ open class IDEEditor languageClient = null _file = null - fileVersion = 0 + fileVersion.set(0) markUnmodified() editorFeatures.editor = null @@ -960,7 +973,9 @@ open class IDEEditor file ?: return@subscribeEvent editorScope.launch { - dispatchDocumentChangeEvent(event) + // Serialised so the version a change is stamped with is never older than the text + // snapshot taken with it: two edits in one frame land here as two coroutines. + documentChangeMutex.withLock { dispatchDocumentChangeEvent(event) } checkForSignatureHelp(event) handleCustomTextReplacement(event) } @@ -1242,9 +1257,9 @@ open class IDEEditor val file = this.file ?: return - this.fileVersion = 0 + this.fileVersion.set(0) - val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion) + val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion.get()) eventDispatcher.dispatch(openEvent) } @@ -1278,7 +1293,7 @@ open class IDEEditor file, changedText, text.toString(), - ++fileVersion, + fileVersion.incrementAndGet(), type, changeDelta, changeRange, diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt index 59d6242568..c19d05f1d9 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt @@ -43,28 +43,19 @@ import java.util.concurrent.ConcurrentHashMap * @author Akash Yadav */ object FileManager { - private val log = LoggerFactory.getLogger(FileManager::class.java) private val _activeDocuments = ConcurrentHashMap() val activeDocuments: Collection get() = _activeDocuments.values.toSet() - fun isActive(uri: URI): Boolean { - return isActive(Paths.get(uri)) - } + fun isActive(uri: URI): Boolean = isActive(Paths.get(uri)) - fun isActive(file: Path): Boolean { - return this._activeDocuments.containsKey(file.normalize()) - } + fun isActive(file: Path): Boolean = this._activeDocuments.containsKey(file.normalize()) - fun getActiveDocument(file: Path): ActiveDocument? { - return this._activeDocuments[file.normalize()] - } + fun getActiveDocument(file: Path): ActiveDocument? = this._activeDocuments[file.normalize()] - fun getActiveDocumentCount(): Int { - return this._activeDocuments.size - } + fun getActiveDocumentCount(): Int = this._activeDocuments.size fun getDocumentContents(file: Path): String { val document = getActiveDocument(file) @@ -115,14 +106,19 @@ object FileManager { _activeDocuments[event.changedFile.normalize()] = createDocument(event) log.warn( "Document change event received before open event for file {}", - event.changedFile + event.changedFile, ) return } - document.version = event.version - document.modified = Instant.now() - document.content = event.newText!! + if (!document.update(event.version, event.newText!!)) { + log.debug( + "Ignoring out-of-order change for {}: event version {} is older than {}", + event.changedFile, + event.version, + document.version, + ) + } event.newText = null } @@ -142,26 +138,24 @@ object FileManager { _activeDocuments.remove(event.file.toPath().normalize()) } - private fun createDocument(event: DocumentOpenEvent): ActiveDocument { - return ActiveDocument( + private fun createDocument(event: DocumentOpenEvent): ActiveDocument = + ActiveDocument( file = event.openedFile, version = event.version, modified = Instant.now(), - content = event.text + content = event.text, ) - } - private fun createDocument(event: DocumentChangeEvent): ActiveDocument { - return ActiveDocument( + private fun createDocument(event: DocumentChangeEvent): ActiveDocument = + ActiveDocument( file = event.changedFile, version = event.version, modified = Instant.now(), - content = event.changedText + content = event.changedText, ) - } - private fun createFileReader(file: Path): BufferedReader { - return try { + private fun createFileReader(file: Path): BufferedReader = + try { Files.newBufferedReader(file) } catch (noFile: java.nio.file.NoSuchFileException) { log.warn("No such file", noFile) @@ -169,10 +163,9 @@ object FileManager { } catch (cancelled: CancellationException) { "".reader().buffered() } - } - private fun createFileInputStream(file: Path): InputStream { - return try { + private fun createFileInputStream(file: Path): InputStream = + try { Files.newInputStream(file) } catch (noFile: java.nio.file.NoSuchFileException) { log.warn("No such file", noFile) @@ -180,14 +173,11 @@ object FileManager { } catch (cancelled: CancellationException) { "".byteInputStream() } - } - private fun getLastModifiedFromDisk(file: Path): Instant { - return Files.getLastModifiedTime(file).toInstant() - } + private fun getLastModifiedFromDisk(file: Path): Instant = Files.getLastModifiedTime(file).toInstant() - private fun getFileContents(file: Path): String { - return try { + private fun getFileContents(file: Path): String = + try { ProgressManager.abortIfCancelled() FileUtils.readFileToString(file.toFile(), Charset.defaultCharset()) } catch (noFile: java.nio.file.NoSuchFileException) { @@ -196,5 +186,4 @@ object FileManager { } catch (cancelled: CancellationException) { "" } - } } diff --git a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt index 42b0b7e6cc..09b28ce6f6 100644 --- a/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt +++ b/subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt @@ -29,19 +29,60 @@ import java.time.Instant */ open class ActiveDocument( val file: Path, - var version: Int, - var modified: Instant, - content: String = "" + version: Int, + modified: Instant, + content: String = "", ) { + private data class Snapshot( + val version: Int, + val modified: Instant, + val content: String, + ) - var content: String = content - internal set + /* + * One volatile reference, so a reader can never pair a new version with the old content. The editor + * dispatches change events from a background coroutine per edit, so two edits in one frame do reach + * this concurrently. + */ + @Volatile + private var snapshot = Snapshot(version, modified, content) - fun inputStream(): BufferedInputStream { - return content.byteInputStream().buffered() - } + /** The version last published via [update]. Always consistent with [content] and [modified]. */ + val version: Int + get() = snapshot.version + + /** The timestamp of the last [update]. Always consistent with [version] and [content]. */ + val modified: Instant + get() = snapshot.modified + + /** The content last published via [update]. Always consistent with [version] and [modified]. */ + val content: String + get() = snapshot.content - fun reader(): BufferedReader { - return content.reader().buffered() + /** + * Publishes [content] at [version], or returns false if [version] is older than what is already + * published. + * + * A version that moves backwards makes the Kotlin index mint a second `KtFile` for text that never + * changed, which is what surfaced as redeclaration errors across a whole file (ADFA-5231). + * + * An equal version is accepted and overwrites, rather than being rejected like an older one. The + * only writer, `IDEEditor`, stamps versions from a single serialised `AtomicInteger.incrementAndGet()` + * per document, so distinct edits never share a version - an equal version is a re-delivery of the + * same edit, and taking its (identical) content is harmless. + */ + internal fun update( + version: Int, + content: String, + ): Boolean { + synchronized(this) { + if (version < snapshot.version) return false + snapshot = Snapshot(version, Instant.now(), content) + return true + } } + + fun inputStream(): BufferedInputStream = content.byteInputStream().buffered() + + fun reader(): BufferedReader = content.reader().buffered() } diff --git a/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt new file mode 100644 index 0000000000..b6efed4f33 --- /dev/null +++ b/subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt @@ -0,0 +1,47 @@ +package com.itsaky.androidide.projects + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.eventbus.events.editor.ChangeType +import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent +import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent +import com.itsaky.androidide.models.Range +import org.junit.After +import org.junit.Test +import java.nio.file.Paths + +/** A document's version and content always move forward together. */ +class ActiveDocumentVersionTest { + private val path = Paths.get("/tmp/adfa5231/Main.kt") + + @After + fun close() { + FileManager.onDocumentClose(DocumentCloseEvent(path)) + } + + private fun change( + text: String, + version: Int, + ) = DocumentChangeEvent(path, text, text, version, ChangeType.NEW_TEXT, 0, Range.NONE) + + @Test + fun `a backwards version is rejected and leaves the newer content in place`() { + FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1)) + FileManager.onDocumentContentChange(change("v3", 3)) + + FileManager.onDocumentContentChange(change("v2", 2)) + + val document = FileManager.getActiveDocument(path)!! + assertThat(document.version).isEqualTo(3) + assertThat(document.content).isEqualTo("v3") + } + + @Test + fun `a version and its content are never observed apart`() { + FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1)) + FileManager.onDocumentContentChange(change("v2", 2)) + + val document = FileManager.getActiveDocument(path)!! + assertThat(document.version to document.content).isEqualTo(2 to "v2") + } +}