[Workspace Model]: IDEA-284866 Fix read/write locks usage

GitOrigin-RevId: 7aa68f1c614b356ae95cc8afd49d2358ae4a82da
2026-02-04 23:39:07 +07:00 · 2021-12-16 16:28:50 +03:00
parent 566e31e2cb
commit cabe230c15
2 changed files with 226 additions and 164 deletions
--- a/java/compiler/impl/src/com/intellij/packaging/impl/artifacts/workspacemodel/EntityToElement.kt
+++ b/java/compiler/impl/src/com/intellij/packaging/impl/artifacts/workspacemodel/EntityToElement.kt
@@ -16,6 +16,7 @@ import com.intellij.workspaceModel.ide.WorkspaceModel
 import com.intellij.workspaceModel.storage.VersionedEntityStorage
 import com.intellij.workspaceModel.storage.WorkspaceEntityStorageBuilder
 import com.intellij.workspaceModel.storage.bridgeEntities.*
+import org.jetbrains.annotations.TestOnly
 import org.jetbrains.jps.util.JpsPathUtil
 import java.util.concurrent.TimeUnit
 import java.util.concurrent.locks.Condition
@@ -34,191 +35,201 @@ internal fun CompositePackagingElementEntity.toCompositeElement(
  addToMapping: Boolean = true,
 ): CompositePackagingElement<*> {
  rwLock.readLock().lock()
-  try {
-    var existing = storage.current.elements.getDataByEntity(this)
-    if (existing == null) {
-      rwLock.readLock().unlock()
-      rwLock.writeLock().lock()
-      try {
-        // Double check
-        existing = storage.current.elements.getDataByEntity(this)
-        if (existing == null) {
-          val element = when (this) {
-            is DirectoryPackagingElementEntity -> {
-              val element = DirectoryPackagingElement(this.directoryName)
-              this.children.pushTo(element, project, storage)
-              element
-            }
-            is ArchivePackagingElementEntity -> {
-              val element = ArchivePackagingElement(this.fileName)
-              this.children.pushTo(element, project, storage)
-              element
-            }
-            is ArtifactRootElementEntity -> {
-              val element = ArtifactRootElementImpl()
-              this.children.pushTo(element, project, storage)
-              element
-            }
-            is CustomPackagingElementEntity -> {
-              val unpacked = unpackCustomElement(storage, project)
-              if (unpacked !is CompositePackagingElement<*>) {
-                error("Expected composite packaging element")
-              }
-              unpacked
-            }
-            else -> unknownElement()
-          }
-          if (addToMapping) {
-            val storageBase = storage.base
-            if (storageBase is WorkspaceEntityStorageBuilder) {
-              val mutableMapping = storageBase.mutableElements
-              mutableMapping.addMapping(this, element)
-            }
-            else {
-              WorkspaceModel.getInstance(project).updateProjectModelSilent {
-                val mutableMapping = it.mutableElements
-                mutableMapping.addMapping(this, element)
-              }
-            }
-          }
-          existing =  element
-        }
-        // Lock downgrade
-        rwLock.readLock().lock()
-      }
-      finally {
-        rwLock.writeLock().unlock()
-      }
-    }

-    return existing as CompositePackagingElement<*>
+  var existing = try {
+    testCheck(1)
+    storage.current.elements.getDataByEntity(this)
  }
-  finally {
+  catch (e: Exception) {
    rwLock.readLock().unlock()
+    throw e
  }
-}
-
-fun PackagingElementEntity.toElement(project: Project, storage: VersionedEntityStorage): PackagingElement<*> {
-  rwLock.readLock().lock()
-  try {
-    var existing = storage.current.elements.getDataByEntity(this)
-    if (existing == null) {
-      rwLock.readLock().unlock()
-      rwLock.writeLock().lock()
-      try {
-        // Double check
-        existing = storage.current.elements.getDataByEntity(this)
-        if (existing == null) {
-          val element = when (this) {
-            is ModuleOutputPackagingElementEntity -> {
-              val module = this.module
-              if (module != null) {
-                val modulePointer = ModulePointerManager.getInstance(project).create(module.name)
-                ProductionModuleOutputPackagingElement(project, modulePointer)
-              }
-              else {
-                ProductionModuleOutputPackagingElement(project)
-              }
-            }
-            is ModuleTestOutputPackagingElementEntity -> {
-              val module = this.module
-              if (module != null) {
-                val modulePointer = ModulePointerManager.getInstance(project).create(module.name)
-                TestModuleOutputPackagingElement(project, modulePointer)
-              }
-              else {
-                TestModuleOutputPackagingElement(project)
-              }
-            }
-            is ModuleSourcePackagingElementEntity -> {
-              val module = this.module
-              if (module != null) {
-                val modulePointer = ModulePointerManager.getInstance(project).create(module.name)
-                ProductionModuleSourcePackagingElement(project, modulePointer)
-              }
-              else {
-                ProductionModuleSourcePackagingElement(project)
-              }
-            }
-            is ArtifactOutputPackagingElementEntity -> {
-              val artifact = this.artifact
-              if (artifact != null) {
-                val artifactPointer = ArtifactPointerManager.getInstance(project).createPointer(artifact.name)
-                ArtifactPackagingElement(project, artifactPointer)
-              }
-              else {
-                ArtifactPackagingElement(project)
-              }
-            }
-            is ExtractedDirectoryPackagingElementEntity -> {
-              val pathInArchive = this.pathInArchive
-              val archive = this.filePath
-              ExtractedDirectoryPackagingElement(JpsPathUtil.urlToPath(archive.url), pathInArchive)
-            }
-            is FileCopyPackagingElementEntity -> {
-              val file = this.filePath
-              val renamedOutputFileName = this.renamedOutputFileName
-              if (renamedOutputFileName != null) {
-                FileCopyPackagingElement(JpsPathUtil.urlToPath(file.url), renamedOutputFileName)
-              }
-              else {
-                FileCopyPackagingElement(JpsPathUtil.urlToPath(file.url))
-              }
-            }
-            is DirectoryCopyPackagingElementEntity -> {
-              val directory = this.filePath
-              DirectoryCopyPackagingElement(JpsPathUtil.urlToPath(directory.url))
-            }
-            is ArchivePackagingElementEntity -> this.toCompositeElement(project, storage, false)
-            is DirectoryPackagingElementEntity -> this.toCompositeElement(project, storage, false)
-            is ArtifactRootElementEntity -> this.toCompositeElement(project, storage, false)
-            is LibraryFilesPackagingElementEntity -> {
-              val mapping = storage.current.getExternalMapping<PackagingElement<*>>("intellij.artifacts.packaging.elements")
-              val data = mapping.getDataByEntity(this)
-              if (data != null) {
-                return data
-              }
-
-              val library = this.library
-              if (library != null) {
-                val tableId = library.tableId
-                val moduleName = if (tableId is LibraryTableId.ModuleLibraryTableId) tableId.moduleId.name else null
-                LibraryPackagingElement(tableId.level, library.name, moduleName)
-              }
-              else {
-                LibraryPackagingElement()
-              }
-            }
-            is CustomPackagingElementEntity -> unpackCustomElement(storage, project)
-            else -> unknownElement()
+  if (existing == null) {
+    rwLock.readLock().unlock()
+    rwLock.writeLock().lock()
+    try {
+      testCheck(2)
+      // Double check
+      existing = storage.current.elements.getDataByEntity(this)
+      if (existing == null) {
+        val element = when (this) {
+          is DirectoryPackagingElementEntity -> {
+            val element = DirectoryPackagingElement(this.directoryName)
+            this.children.pushTo(element, project, storage)
+            element
          }
-
+          is ArchivePackagingElementEntity -> {
+            val element = ArchivePackagingElement(this.fileName)
+            this.children.pushTo(element, project, storage)
+            element
+          }
+          is ArtifactRootElementEntity -> {
+            val element = ArtifactRootElementImpl()
+            this.children.pushTo(element, project, storage)
+            element
+          }
+          is CustomPackagingElementEntity -> {
+            val unpacked = unpackCustomElement(storage, project)
+            if (unpacked !is CompositePackagingElement<*>) {
+              error("Expected composite packaging element")
+            }
+            unpacked
+          }
+          else -> unknownElement()
+        }
+        if (addToMapping) {
          val storageBase = storage.base
          if (storageBase is WorkspaceEntityStorageBuilder) {
            val mutableMapping = storageBase.mutableElements
-            mutableMapping.addIfAbsent(this, element)
+            mutableMapping.addMapping(this, element)
          }
          else {
            WorkspaceModel.getInstance(project).updateProjectModelSilent {
              val mutableMapping = it.mutableElements
-              mutableMapping.addIfAbsent(this, element)
+              mutableMapping.addMapping(this, element)
            }
          }
-          existing = element
        }
-        // Lock downgrade
-        rwLock.readLock().lock()
-      }
-      finally {
-        rwLock.writeLock().unlock()
+        existing = element
      }
+      // Lock downgrade
+      rwLock.readLock().lock()
    }
+    finally {
+      rwLock.writeLock().unlock()
+    }
+  }

-    return existing as PackagingElement<*>
+  rwLock.readLock().unlock()
+  return existing as CompositePackagingElement<*>
+}
+
+fun PackagingElementEntity.toElement(project: Project, storage: VersionedEntityStorage): PackagingElement<*> {
+  rwLock.readLock().lock()
+
+  var existing = try {
+    testCheck(3)
+    storage.current.elements.getDataByEntity(this)
  }
-  finally {
+  catch (e: Exception) {
    rwLock.readLock().unlock()
+    throw e
  }
+  if (existing == null) {
+    rwLock.readLock().unlock()
+    rwLock.writeLock().lock()
+    try {
+      testCheck(4)
+      // Double check
+      existing = storage.current.elements.getDataByEntity(this)
+      if (existing == null) {
+        val element = when (this) {
+          is ModuleOutputPackagingElementEntity -> {
+            val module = this.module
+            if (module != null) {
+              val modulePointer = ModulePointerManager.getInstance(project).create(module.name)
+              ProductionModuleOutputPackagingElement(project, modulePointer)
+            }
+            else {
+              ProductionModuleOutputPackagingElement(project)
+            }
+          }
+          is ModuleTestOutputPackagingElementEntity -> {
+            val module = this.module
+            if (module != null) {
+              val modulePointer = ModulePointerManager.getInstance(project).create(module.name)
+              TestModuleOutputPackagingElement(project, modulePointer)
+            }
+            else {
+              TestModuleOutputPackagingElement(project)
+            }
+          }
+          is ModuleSourcePackagingElementEntity -> {
+            val module = this.module
+            if (module != null) {
+              val modulePointer = ModulePointerManager.getInstance(project).create(module.name)
+              ProductionModuleSourcePackagingElement(project, modulePointer)
+            }
+            else {
+              ProductionModuleSourcePackagingElement(project)
+            }
+          }
+          is ArtifactOutputPackagingElementEntity -> {
+            val artifact = this.artifact
+            if (artifact != null) {
+              val artifactPointer = ArtifactPointerManager.getInstance(project).createPointer(artifact.name)
+              ArtifactPackagingElement(project, artifactPointer)
+            }
+            else {
+              ArtifactPackagingElement(project)
+            }
+          }
+          is ExtractedDirectoryPackagingElementEntity -> {
+            val pathInArchive = this.pathInArchive
+            val archive = this.filePath
+            ExtractedDirectoryPackagingElement(JpsPathUtil.urlToPath(archive.url), pathInArchive)
+          }
+          is FileCopyPackagingElementEntity -> {
+            val file = this.filePath
+            val renamedOutputFileName = this.renamedOutputFileName
+            if (renamedOutputFileName != null) {
+              FileCopyPackagingElement(JpsPathUtil.urlToPath(file.url), renamedOutputFileName)
+            }
+            else {
+              FileCopyPackagingElement(JpsPathUtil.urlToPath(file.url))
+            }
+          }
+          is DirectoryCopyPackagingElementEntity -> {
+            val directory = this.filePath
+            DirectoryCopyPackagingElement(JpsPathUtil.urlToPath(directory.url))
+          }
+          is ArchivePackagingElementEntity -> this.toCompositeElement(project, storage, false)
+          is DirectoryPackagingElementEntity -> this.toCompositeElement(project, storage, false)
+          is ArtifactRootElementEntity -> this.toCompositeElement(project, storage, false)
+          is LibraryFilesPackagingElementEntity -> {
+            val mapping = storage.current.getExternalMapping<PackagingElement<*>>("intellij.artifacts.packaging.elements")
+            val data = mapping.getDataByEntity(this)
+            if (data != null) {
+              return data
+            }
+
+            val library = this.library
+            if (library != null) {
+              val tableId = library.tableId
+              val moduleName = if (tableId is LibraryTableId.ModuleLibraryTableId) tableId.moduleId.name else null
+              LibraryPackagingElement(tableId.level, library.name, moduleName)
+            }
+            else {
+              LibraryPackagingElement()
+            }
+          }
+          is CustomPackagingElementEntity -> unpackCustomElement(storage, project)
+          else -> unknownElement()
+        }
+
+        val storageBase = storage.base
+        if (storageBase is WorkspaceEntityStorageBuilder) {
+          val mutableMapping = storageBase.mutableElements
+          mutableMapping.addIfAbsent(this, element)
+        }
+        else {
+          WorkspaceModel.getInstance(project).updateProjectModelSilent {
+            val mutableMapping = it.mutableElements
+            mutableMapping.addIfAbsent(this, element)
+          }
+        }
+        existing = element
+      }
+      // Lock downgrade
+      rwLock.readLock().lock()
+    }
+    finally {
+      rwLock.writeLock().unlock()
+    }
+  }
+
+  rwLock.readLock().unlock()
+  return existing as PackagingElement<*>
 }

 private fun CustomPackagingElementEntity.unpackCustomElement(storage: VersionedEntityStorage,
@@ -289,3 +300,24 @@ private object EmptyLock : Lock {
    throw UnsupportedOperationException()
  }
 }
+
+// Instruments for testing code with unexpected exceptions
+// I assume that tests with bad approach is better than no tests at all
+@TestOnly
+object ArtifactsTestingState {
+  var testLevel: Int = 0
+  var exceptionsThrows: MutableList<Int> = ArrayList()
+
+  fun reset() {
+    testLevel = 0
+    exceptionsThrows = ArrayList()
+  }
+}
+
+@Suppress("TestOnlyProblems")
+private fun testCheck(level: Int) {
+  if (level == ArtifactsTestingState.testLevel) {
+    ArtifactsTestingState.exceptionsThrows += level
+    error("Exception on level: $level")
+  }
+}
--- a/java/compiler/tests/com/intellij/compiler/artifacts/workspaceModel/ArtifactTest.kt
+++ b/java/compiler/tests/com/intellij/compiler/artifacts/workspaceModel/ArtifactTest.kt
@@ -19,6 +19,7 @@ import com.intellij.packaging.elements.PackagingElementType
 import com.intellij.packaging.impl.artifacts.InvalidArtifact
 import com.intellij.packaging.impl.artifacts.PlainArtifactType
 import com.intellij.packaging.impl.artifacts.workspacemodel.ArtifactManagerBridge.Companion.artifactsMap
+import com.intellij.packaging.impl.artifacts.workspacemodel.ArtifactsTestingState
 import com.intellij.packaging.impl.artifacts.workspacemodel.forThisAndFullTree
 import com.intellij.packaging.impl.artifacts.workspacemodel.toElement
 import com.intellij.packaging.impl.elements.ArtifactRootElementImpl
@@ -35,6 +36,11 @@ import java.util.concurrent.Callable

 class ArtifactTest : ArtifactsTestCase() {

+  override fun tearDown() {
+    ArtifactsTestingState.reset()
+    super.tearDown()
+  }
+
  fun `test rename artifact via model`() = runWriteAction {
    assumeTrue(WorkspaceModel.enabledForArtifacts)

@@ -412,6 +418,30 @@ class ArtifactTest : ArtifactsTestCase() {
    }
  }

+  fun `test artifacts with exceptions during initialization`() {
+    assumeTrue(WorkspaceModel.enabledForArtifacts)
+
+    var exceptionsThrown: List<Int> = emptyList()
+    repeat(4) {
+      val rootEntity = runWriteAction {
+        WorkspaceModel.getInstance(project).updateProjectModel {
+          it.addArtifactRootElementEntity(emptyList(), MySource)
+        }
+      }
+      ArtifactsTestingState.testLevel = it + 1
+      try {
+        rootEntity.toElement(project, WorkspaceModel.getInstance(project).entityStorage)
+      } catch (e: IllegalStateException) {
+        if (e.message?.contains("Exception on level") != true) {
+          error("Unexpected exception")
+        }
+      }
+
+      exceptionsThrown = ArtifactsTestingState.exceptionsThrows
+    }
+    TestCase.assertEquals(listOf(1, 2, 3, 4), exceptionsThrown)
+  }
+
  fun `test async artifacts requesting`() {
    assumeTrue(WorkspaceModel.enabledForArtifacts)