isolate SchemeLoader — part 2: don't touch schemeToInfo and schemes

platform/configuration-store-impl/src/schemeManager/SchemeListManager.kt

@@ -118,26 +118,4 @@ internal class SchemeListManager<T : Any>(private val schemeManager: SchemeManag
     schemes.mapTo(result) { schemeManager.processor.getSchemeKey(it) }
     return result
   }
-
-  fun removeFirstScheme(schemes: MutableList<T>, scheduleDelete: Boolean = true, condition: (T) -> Boolean): T? {
-    val iterator = schemes.iterator()
-    for (scheme in iterator) {
-      if (!condition(scheme)) {
-        continue
-      }
-
-      if (schemeManager.activeScheme === scheme) {
-        schemeManager.activeScheme = null
-      }
-
-      iterator.remove()
-
-      if (scheduleDelete && schemeManager.processor.isExternalizable(scheme)) {
-        schemeManager.schemeToInfo.remove(scheme)?.let(schemeManager::scheduleDelete)
-      }
-      return scheme
-    }
-
-    return null
-  }
 }

platform/configuration-store-impl/src/schemeManager/SchemeManagerImpl.kt

@@ -132,8 +132,7 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
 
         externalInfo.schemeKey = schemeKey
 
-        val scheme = (processor as LazySchemeProcessor).createScheme(
-          SchemeDataHolderImpl(processor, bytes, externalInfo), schemeKey, attributeProvider, true)
+        val scheme = (processor as LazySchemeProcessor).createScheme(SchemeDataHolderImpl(processor, bytes, externalInfo), schemeKey, attributeProvider, true)
         val oldInfo = schemeToInfo.put(scheme, externalInfo)
         LOG.assertTrue(oldInfo == null)
         val oldScheme = schemeListManager.readOnlyExternalizableSchemes.put(schemeKey, scheme)
@@ -559,15 +558,38 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
 
   override fun findSchemeByName(schemeName: String) = schemes.firstOrNull { processor.getSchemeKey(it) == schemeName }
 
-  override fun removeScheme(name: String) = schemeListManager.removeFirstScheme(schemes) {processor.getSchemeKey(it) == name }
+  override fun removeScheme(name: String) = removeFirstScheme(schemes, this) {processor.getSchemeKey(it) == name }
 
-  override fun removeScheme(scheme: T) = schemeListManager.removeFirstScheme(schemes) { it == scheme } != null
+  override fun removeScheme(scheme: T) = removeFirstScheme(schemes, this) { it == scheme } != null
 
   override fun isMetadataEditable(scheme: T) = !schemeListManager.readOnlyExternalizableSchemes.containsKey(processor.getSchemeKey(scheme))
 
   override fun toString() = fileSpec
 }
 
+// static method to ensure that receiver state is not used
+private fun <T : Any> removeFirstScheme(schemes: MutableList<T>, schemeManager: SchemeManagerImpl<T, *>, condition: (T) -> Boolean): T? {
+  val iterator = schemes.iterator()
+  for (scheme in iterator) {
+    if (!condition(scheme)) {
+      continue
+    }
+
+    if (schemeManager.activeScheme === scheme) {
+      schemeManager.activeScheme = null
+    }
+
+    iterator.remove()
+
+    if (schemeManager.processor.isExternalizable(scheme)) {
+      schemeManager.schemeToInfo.remove(scheme)?.let(schemeManager::scheduleDelete)
+    }
+    return scheme
+  }
+
+  return null
+}
+
 internal fun nameIsMissed(bytes: ByteArray): RuntimeException {
   return RuntimeException("Name is missed:\n${bytes.toString(Charsets.UTF_8)}")
 }

platform/configuration-store-impl/src/schemeManager/schemeLoader.kt

@@ -10,6 +10,7 @@ import com.intellij.openapi.util.Ref
 import com.intellij.openapi.vfs.VfsUtil
 import com.intellij.openapi.vfs.VirtualFile
 import com.intellij.util.containers.ConcurrentList
+import com.intellij.util.containers.ContainerUtil
 import com.intellij.util.io.createDirectories
 import com.intellij.util.io.systemIndependentPath
 import gnu.trove.THashSet
@@ -31,20 +32,28 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
   private val filesToDelete = THashSet<String>()
 
   private val schemes = oldSchemes.toMutableList()
-  private val newSchemesOffset = schemes.size
+  private var newSchemesOffset = schemes.size
+
+  // scheme could be changed - so, hashcode will be changed - we must use identity hashing strategy
+  private val schemeToInfo = ContainerUtil.newIdentityTroveMap<T, ExternalInfo>()
 
   private val isApplied = AtomicBoolean()
 
   private var digest: MessageDigest? = null
 
+  // or from current session, or from current state
+  private fun getInfoForExistingScheme(existingScheme: T): ExternalInfo? {
+    return schemeToInfo.get(existingScheme) ?: schemeManager.schemeToInfo.get(existingScheme)
+  }
+
   private fun isFromFileWithOldExtension(existingScheme: T): Boolean {
-    val info = schemeManager.schemeToInfo.get(existingScheme)
+    val info = getInfoForExistingScheme(existingScheme)
     // scheme from file with old extension, so, we must ignore it
     return info != null && schemeManager.schemeExtension != info.fileExtension
   }
 
   private fun isFromFileWithNewExtension(existingScheme: T, fileNameWithoutExtension: String): Boolean {
-    return schemeManager.schemeToInfo.get(existingScheme)?.fileNameWithoutExtension == fileNameWithoutExtension
+    return getInfoForExistingScheme(existingScheme)?.fileNameWithoutExtension == fileNameWithoutExtension
   }
 
   /**
@@ -52,10 +61,11 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
    */
   fun apply(): List<T> {
     LOG.assertTrue(isApplied.compareAndSet(false, true))
-
     schemeManager.filesToDelete.addAll(filesToDelete)
     schemeManager.filesToDelete.addAll(preScheduledFilesToDelete)
 
+    schemeManager.schemeToInfo.putAll(schemeToInfo)
+
     val result = schemes.subList(newSchemesOffset, schemes.size)
     schemeManager.schemeListManager.replaceSchemeList(oldSchemes, schemes)
     if (!isDuringLoad) {
@@ -79,6 +89,44 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
     return result
   }
 
+  private fun checkExisting(schemeKey: String, fileName: String, fileNameWithoutExtension: String, extension: String): Boolean {
+    val processor = schemeManager.processor
+    // schemes load session doesn't care about any scheme that added after session creation,
+    // e.g. for now, on apply, simply current manager list replaced atomically to the new one
+    // if later it will lead to some issues, this check should be done as merge operation (again, currently on apply old list is replaced and not merged)
+    val existingSchemeIndex = schemes.indexOfFirst { processor.getSchemeKey(it) == schemeKey }
+    val existingScheme = (if (existingSchemeIndex == -1) null else schemes.get(existingSchemeIndex)) ?: return true
+    if (schemeManager.schemeListManager.readOnlyExternalizableSchemes.get(processor.getSchemeKey(existingScheme)) === existingScheme) {
+      // so, bundled scheme is shadowed
+      schemes.removeAt(existingSchemeIndex)
+      if (existingSchemeIndex < newSchemesOffset) {
+        newSchemesOffset--
+      }
+      // not added to filesToDelete because it is only shadowed
+      return true
+    }
+    else if (processor.isExternalizable(existingScheme) && isFromFileWithOldExtension(existingScheme)) {
+      schemes.removeAt(existingSchemeIndex)
+      if (existingSchemeIndex < newSchemesOffset) {
+        newSchemesOffset--
+      }
+      filesToDelete.add(fileName)
+    }
+    else {
+      if (schemeManager.schemeExtension != extension && isFromFileWithNewExtension(existingScheme, fileNameWithoutExtension)) {
+        // 1.oldExt is loading after 1.newExt - we should delete 1.oldExt
+        filesToDelete.add(fileName)
+      }
+      else {
+        // We don't load scheme with duplicated name - if we generate unique name for it, it will be saved then with new name.
+        // It is not what all can expect. Such situation in most cases indicates error on previous level, so, we just warn about it.
+        LOG.warn("Scheme file \"$fileName\" is not loaded because defines duplicated name \"$schemeKey\"")
+      }
+      return false
+    }
+    return true
+  }
+
   fun loadScheme(fileName: String, input: InputStream): MUTABLE_SCHEME? {
     val extension = schemeManager.getFileExtension(fileName, isAllowAny = false)
     if (isFileScheduledForDeleteInThisLoadSession(fileName)) {
@@ -87,37 +135,8 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
     }
 
     val processor = schemeManager.processor
-    val schemeListManager = schemeManager.schemeListManager
-
     val fileNameWithoutExtension = fileName.substring(0, fileName.length - extension.length)
 
-    fun checkExisting(schemeName: String): Boolean {
-      schemes.firstOrNull { processor.getSchemeKey(it) == schemeName}?.let { existingScheme ->
-        if (schemeListManager.readOnlyExternalizableSchemes.get(processor.getSchemeKey(existingScheme)) === existingScheme) {
-          // so, bundled scheme is shadowed
-          schemeListManager.removeFirstScheme(schemes, scheduleDelete = false) { it === existingScheme }
-          return true
-        }
-        else if (processor.isExternalizable(existingScheme) && isFromFileWithOldExtension(existingScheme)) {
-          schemeListManager.removeFirstScheme(schemes) { it === existingScheme }
-        }
-        else {
-          if (schemeManager.schemeExtension != extension && isFromFileWithNewExtension(existingScheme, fileNameWithoutExtension)) {
-            // 1.oldExt is loading after 1.newExt - we should delete 1.oldExt
-            filesToDelete.add(fileName)
-          }
-          else {
-            // We don't load scheme with duplicated name - if we generate unique name for it, it will be saved then with new name.
-            // It is not what all can expect. Such situation in most cases indicates error on previous level, so, we just warn about it.
-            LOG.warn("Scheme file \"$fileName\" is not loaded because defines duplicated name \"$schemeName\"")
-          }
-          return false
-        }
-      }
-
-      return true
-    }
-
     fun createInfo(schemeName: String, element: Element?): ExternalInfo {
       val info = ExternalInfo(fileNameWithoutExtension, extension)
       if (element != null) {
@@ -141,16 +160,16 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
             null
           }
         }
-        val schemeName = name
-                         ?: processor.getSchemeKey(attributeProvider, fileNameWithoutExtension)
-                         ?: throw nameIsMissed(bytes)
-        if (!checkExisting(schemeName)) {
+        val schemeKey = name
+                        ?: processor.getSchemeKey(attributeProvider, fileNameWithoutExtension)
+                        ?: throw nameIsMissed(bytes)
+        if (!checkExisting(schemeKey, fileName, fileNameWithoutExtension, extension)) {
           return null
         }
 
-        val externalInfo = createInfo(schemeName, null)
-        scheme = processor.createScheme(SchemeDataHolderImpl(processor, bytes, externalInfo), schemeName, attributeProvider)
-        schemeManager.schemeToInfo.put(scheme, externalInfo)
+        val externalInfo = createInfo(schemeKey, null)
+        scheme = processor.createScheme(SchemeDataHolderImpl(processor, bytes, externalInfo), schemeKey, attributeProvider)
+        schemeToInfo.put(scheme, externalInfo)
         retainProbablyScheduledForDeleteFile(fileName)
       }
     }
@@ -158,11 +177,11 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
       val element = JDOMUtil.load(input.bufferedReader())
       scheme = (processor as NonLazySchemeProcessor).readScheme(element, isDuringLoad) ?: return null
       val schemeKey = processor.getSchemeKey(scheme!!)
-      if (!checkExisting(schemeKey)) {
+      if (!checkExisting(schemeKey, fileName, fileNameWithoutExtension, extension)) {
         return null
       }
 
-      schemeManager.schemeToInfo.put(scheme, createInfo(schemeKey, element))
+      schemeToInfo.put(scheme, createInfo(schemeKey, element))
       retainProbablyScheduledForDeleteFile(fileName)
     }