fix: dont freeze editor when closing editor with offline cluster

Freezing was caused by race conditions in the locks. Closing the editor
disposes the editor resources. Disposing the editor resource closes the
cluster connection. When closing the watch, a new resource state is
retrieved, this then ended in the resource lock waiting for the
connection timeout.
Solved it by introducing a new "Disposed" state which is returned
whenever an editor resource is disposed. No further state retrieval
occurrs.

Signed-off-by: Andre Dietisheim <[email protected]>

Author: adietish

src/main/kotlin/com/redhat/devtools/intellij/kubernetes/editor/EditorResource.kt

@@ -24,6 +24,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata
 import io.fabric8.kubernetes.client.KubernetesClient
 import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil
 import io.fabric8.kubernetes.client.utils.Serialization
+import java.util.concurrent.atomic.AtomicBoolean
 import java.util.concurrent.locks.ReentrantLock
 import kotlin.concurrent.withLock
 
@@ -39,7 +40,7 @@ open class EditorResource(
     ) -> ClusterResource? =
         ClusterResource.Factory::create
 ) {
-    var disposed: Boolean = false
+    var disposed: AtomicBoolean = AtomicBoolean(false)
     /** for testing purposes */
     private var state: EditorResourceState? = null
     /** for testing purposes */
@@ -94,6 +95,9 @@ open class EditorResource(
 
     /** for testing purposes */
     open fun setState(state: EditorResourceState?) {
+        if (disposed.get()) {
+            return
+        }
         resourceChangeMutex.withLock {
             this.state = state
         }
@@ -108,14 +112,18 @@ open class EditorResource(
      * @see [createState]
      */
     fun getState(): EditorResourceState {
-        return resourceChangeMutex.withLock {
-            val existingState = this.state
-            if (existingState == null) {
-                val newState = createState(resource, null)
-                setState(newState)
-                newState
-            } else {
-                existingState
+        return if (disposed.get()) {
+            Disposed()
+        } else {
+            resourceChangeMutex.withLock {
+                val existingState = this.state
+                if (existingState == null) {
+                    val newState = createState(resource, null)
+                    setState(newState)
+                    newState
+                } else {
+                    existingState
+                }
             }
         }
     }
@@ -262,7 +270,7 @@ open class EditorResource(
     /** for testing purposes */
     protected open fun setLastPushedPulled(resource: HasMetadata?) {
         resourceChangeMutex.withLock {
-            lastPushedPulled = resource
+            this.lastPushedPulled = resource
         }
     }
 
@@ -332,13 +340,13 @@ open class EditorResource(
     }
 
     fun dispose() {
-        if (disposed) {
+        if (disposed.get()) {
             return
         }
-        this.disposed = true
-        clusterResource?.close()
+        this.disposed.set(true)
         setLastPushedPulled(null)
         setResourceVersion(null)
+        clusterResource?.close()
     }
 
     private fun createClusterResource(resource: HasMetadata): ClusterResource? {

src/main/kotlin/com/redhat/devtools/intellij/kubernetes/editor/EditorResourceState.kt

@@ -63,6 +63,20 @@ class Error(val title: String, val message: String? = null): EditorResourceState
     }
 }
 
+class Disposed: EditorResourceState() {
+
+    override fun equals(other: Any?): Boolean {
+        if (this === other) {
+            return true
+        }
+        return other is Disposed
+    }
+
+    override fun hashCode(): Int {
+        return Objects.hash()
+    }
+}
+
 open class Identical: EditorResourceState()
 
 abstract class Different(val exists: Boolean, val isOutdatedVersion: Boolean): EditorResourceState() {

src/main/kotlin/com/redhat/devtools/intellij/kubernetes/editor/EditorResources.kt

@@ -59,7 +59,7 @@ open class EditorResources(
                     // remove editor resource for old resource that doesn't exist anymore
                     !new.contains(identifier)
                             // or editor resource that was disposed (ex. change in namespace, context)
-                            || editorResource.disposed
+                            || editorResource.disposed.get()
                 }
             resources.keys.removeAll(toRemove.keys)
             toRemove.values.forEach { editorResource ->

src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/ResourceWatch.kt

@@ -11,11 +11,13 @@
 package com.redhat.devtools.intellij.kubernetes.model
 
 import com.intellij.openapi.diagnostic.logger
+import com.intellij.util.concurrency.AppExecutorUtil
 import io.fabric8.kubernetes.api.model.HasMetadata
 import io.fabric8.kubernetes.client.KubernetesClientException
 import io.fabric8.kubernetes.client.Watch
 import io.fabric8.kubernetes.client.Watcher
 import io.fabric8.kubernetes.client.WatcherException
+import org.jetbrains.concurrency.runAsync
 import java.util.*
 import java.util.concurrent.BlockingDeque
 import java.util.concurrent.ConcurrentHashMap
@@ -29,15 +31,15 @@ import java.util.concurrent.LinkedBlockingDeque
  */
 open class ResourceWatch<T>(
     protected val watchOperations: BlockingDeque<WatchOperation<*>> = LinkedBlockingDeque(),
-    watchOperationsRunner: Runnable = WatchOperationsRunner(watchOperations)
+    watchOperationsRunner: Runnable = WatchOperationsRunner(watchOperations),
+    private val executor: (runnable: Runnable) -> Unit = { runnable -> AppExecutorUtil.getAppExecutorService().execute(runnable) }
 ) {
     companion object {
         @JvmField val WATCH_OPERATION_ENQUEUED: Watch = Watch {  }
     }
 
     protected open val watches: ConcurrentHashMap<T, Watch?> = ConcurrentHashMap()
-    private val executor: ExecutorService = Executors.newWorkStealingPool(20)
-    private val thread = executor.submit(watchOperationsRunner)
+    private val thread = executor.invoke(watchOperationsRunner)
 
     open fun watchAll(
         toWatch: Collection<Pair<T, (watcher: Watcher<in HasMetadata>) -> Watch?>>,
@@ -91,7 +93,9 @@ open class ResourceWatch<T>(
     }
 
     fun close() {
-        closeAll(watches.entries.toList())
+        executor.invoke {
+            closeAll(watches.entries.toList())
+        }
     }
 
     private fun closeAll(entries: Collection<MutableMap.MutableEntry<T, Watch?>>) {

src/test/kotlin/com/redhat/devtools/intellij/kubernetes/editor/EditorResourceStateTest.kt

@@ -114,6 +114,8 @@ class EditorResourceStateTest {
         assertThat(Error("yoda", "is a green gnome")).isNotEqualTo(Error("yoda", "is a jedi"))
         assertThat(Error("obiwan", "is a jedi")).isNotEqualTo(Error("yoda", "is a jedi"))
 
+        assertThat(Error("obiwan", "is a jedi")).isNotEqualTo(Disposed())
+
         assertThat(Error("yoda", "is a jedi")).isNotEqualTo(Identical())
 
         assertThat(Error("yoda", "is a jedi")).isNotEqualTo(Modified(true, true))
@@ -133,12 +135,41 @@ class EditorResourceStateTest {
         assertThat(Error("yoda", "is a jedi")).isNotEqualTo(Pulled())
     }
 
+    @Test
+    fun `Disposed is not equal to all the other states`() {
+        // given
+        // when
+        assertThat(Disposed()).isNotEqualTo(Error("yoda", "is a jedi"))
+
+        assertThat(Disposed()).isEqualTo(Disposed())
+
+        assertThat(Disposed()).isNotEqualTo(Identical())
+
+        assertThat(Disposed()).isNotEqualTo(Modified(true, true))
+        assertThat(Disposed()).isNotEqualTo(Modified(true, false))
+        assertThat(Disposed()).isNotEqualTo(Modified(false, false))
+
+        assertThat(Disposed()).isNotEqualTo(DeletedOnCluster())
+
+        assertThat(Disposed()).isNotEqualTo(Outdated())
+
+        assertThat(Disposed()).isNotEqualTo(Created(true))
+        assertThat(Disposed()).isNotEqualTo(Created(false))
+
+        assertThat(Disposed()).isNotEqualTo(Updated(true))
+        assertThat(Disposed()).isNotEqualTo(Updated(false))
+
+        assertThat(Disposed()).isNotEqualTo(Pulled())
+    }
+
     @Test
     fun `Identical is not equal to all the other states`() {
         // given
         // when
         assertThat(Identical()).isNotEqualTo(Error("yoda", "is a jedi"))
 
+        assertThat(Identical()).isNotEqualTo(Disposed())
+
         assertThat(Identical()).isEqualTo(Identical())
 
         assertThat(Identical()).isNotEqualTo(Modified(true, true))
@@ -167,6 +198,10 @@ class EditorResourceStateTest {
         assertThat(Modified(false, false)).isNotEqualTo(Error("yoda", "is a jedi"))
         assertThat(Modified(false, true)).isNotEqualTo(Error("yoda", "is a jedi"))
 
+        assertThat(Modified(true, true)).isNotEqualTo(Disposed())
+        assertThat(Modified(false, true)).isNotEqualTo(Disposed())
+        assertThat(Modified(false, false)).isNotEqualTo(Disposed())
+
         assertThat(Modified(true, true)).isNotEqualTo(Identical())
         assertThat(Modified(true, false)).isNotEqualTo(Identical())
         assertThat(Modified(false, false)).isNotEqualTo(Identical())
@@ -217,6 +252,8 @@ class EditorResourceStateTest {
         // when
         assertThat(DeletedOnCluster()).isNotEqualTo(Error("darth vader", "is on the dark side"))
 
+        assertThat(DeletedOnCluster()).isNotEqualTo(Disposed())
+
         assertThat(DeletedOnCluster()).isNotEqualTo(Identical())
 
         assertThat(DeletedOnCluster()).isNotEqualTo(Modified(true, true))
@@ -243,6 +280,8 @@ class EditorResourceStateTest {
         // when
         assertThat(Outdated()).isNotEqualTo(Error("obiwan", "is a jedi"))
 
+        assertThat(Outdated()).isNotEqualTo(Disposed())
+
         assertThat(Outdated()).isNotEqualTo(Identical())
 
         assertThat(Outdated()).isNotEqualTo(Modified(true, true))
@@ -270,6 +309,8 @@ class EditorResourceStateTest {
         assertThat(Created(true)).isNotEqualTo(Error("darth vader", "is on the dark side"))
         assertThat(Created(false)).isNotEqualTo(Error("darth vader", "is on the dark side"))
 
+        assertThat(Created(false)).isNotEqualTo(Disposed())
+
         assertThat(Created(true)).isNotEqualTo(Identical())
         assertThat(Created(false)).isNotEqualTo(Identical())
 
@@ -309,6 +350,9 @@ class EditorResourceStateTest {
         assertThat(Updated(true)).isNotEqualTo(Error("darth vader", "is on the dark side"))
         assertThat(Updated(false)).isNotEqualTo(Error("darth vader", "is on the dark side"))
 
+        assertThat(Updated(true)).isNotEqualTo(Disposed())
+        assertThat(Updated(false)).isNotEqualTo(Disposed())
+
         assertThat(Updated(true)).isNotEqualTo(Identical())
         assertThat(Updated(false)).isNotEqualTo(Identical())
 
@@ -347,6 +391,8 @@ class EditorResourceStateTest {
         // when
         assertThat(Pulled()).isNotEqualTo(Error("darth vader", "is on the dark side"))
 
+        assertThat(Pulled()).isNotEqualTo(Disposed())
+
         assertThat(Pulled()).isNotEqualTo(Identical())
 
         assertThat(Pulled()).isNotEqualTo(Modified(true, true))

src/test/kotlin/com/redhat/devtools/intellij/kubernetes/editor/EditorResourceTest.kt

@@ -513,6 +513,29 @@ class EditorResourceTest {
         assertThat(state).isInstanceOf(Error::class.java)
     }
 
+    @Test
+    fun `#getState should return Disposed if resource is disposed`() {
+        // given
+        val editorResource = createEditorResource(POD2)
+        editorResource.disposed.set(true)
+        val state = editorResource.getState()
+        // then
+        assertThat(state).isInstanceOf(Disposed::class.java)
+    }
+
+    @Test
+    fun `#getState should return Disposed even if different state was set after disposal`() {
+        // given
+        val editorResource = createEditorResource(POD2)
+        editorResource.disposed.set(true)
+        val shouldBeIgnored = mock<Error>()
+        editorResource.setState(shouldBeIgnored)
+        // when
+        val state = editorResource.getState()
+        // then
+        assertThat(state).isInstanceOf(Disposed::class.java)
+    }
+
     @Test
     fun `#dispose should close clusterResource that was created`() {
         // given

src/test/kotlin/com/redhat/devtools/intellij/kubernetes/editor/EditorResourcesTest.kt

@@ -33,6 +33,7 @@ import java.util.*
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.After
 import org.junit.Test
+import java.util.concurrent.atomic.AtomicBoolean
 
 class EditorResourcesTest {
 
@@ -230,8 +231,10 @@ class EditorResourcesTest {
     }
 
     private fun createEditorResourceMock(resource: HasMetadata): EditorResource {
+        val disposed = AtomicBoolean(false)
         val editorResource: EditorResource = mock {
-            on { getResource() } doReturn resource
+            on { mock.getResource() } doReturn resource
+            on { mock.disposed } doReturn disposed
         }
         // store editorResource mock in list
         editorResources.add(editorResource)

src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/ResourceWatchTest.kt

@@ -222,7 +222,11 @@ class ResourceWatchTest {
     class TestableResourceWatch(
         watchOperations: BlockingDeque<WatchOperation<*>>,
         watchOperationsRunner: Runnable = mock()
-    ): ResourceWatch<ResourceKind<out HasMetadata>>(watchOperations, watchOperationsRunner) {
+    ) : ResourceWatch<ResourceKind<out HasMetadata>>(
+        watchOperations,
+        watchOperationsRunner,
+        { runnable: Runnable -> runnable.run() }) {
+
         public override val watches = spy(super.watches)
 
         override fun watch(