Skip to content

Commit a3c0073

Browse files
adietishadietish
committed
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]>
1 parent e8f76cf commit a3c0073

File tree

5 files changed

+105
-14
lines changed

5 files changed

+105
-14
lines changed

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

+21-13
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata
2424
import io.fabric8.kubernetes.client.KubernetesClient
2525
import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil
2626
import io.fabric8.kubernetes.client.utils.Serialization
27+
import java.util.concurrent.atomic.AtomicBoolean
2728
import java.util.concurrent.locks.ReentrantLock
2829
import kotlin.concurrent.withLock
2930

@@ -39,7 +40,7 @@ open class EditorResource(
3940
) -> ClusterResource? =
4041
ClusterResource.Factory::create
4142
) {
42-
var disposed: Boolean = false
43+
var disposed: AtomicBoolean = AtomicBoolean(false)
4344
/** for testing purposes */
4445
private var state: EditorResourceState? = null
4546
/** for testing purposes */
@@ -94,6 +95,9 @@ open class EditorResource(
9495

9596
/** for testing purposes */
9697
open fun setState(state: EditorResourceState?) {
98+
if (disposed.get()) {
99+
return
100+
}
97101
resourceChangeMutex.withLock {
98102
this.state = state
99103
}
@@ -108,14 +112,18 @@ open class EditorResource(
108112
* @see [createState]
109113
*/
110114
fun getState(): EditorResourceState {
111-
return resourceChangeMutex.withLock {
112-
val existingState = this.state
113-
if (existingState == null) {
114-
val newState = createState(resource, null)
115-
setState(newState)
116-
newState
117-
} else {
118-
existingState
115+
return if (disposed.get()) {
116+
Disposed()
117+
} else {
118+
resourceChangeMutex.withLock {
119+
val existingState = this.state
120+
if (existingState == null) {
121+
val newState = createState(resource, null)
122+
setState(newState)
123+
newState
124+
} else {
125+
existingState
126+
}
119127
}
120128
}
121129
}
@@ -262,7 +270,7 @@ open class EditorResource(
262270
/** for testing purposes */
263271
protected open fun setLastPushedPulled(resource: HasMetadata?) {
264272
resourceChangeMutex.withLock {
265-
lastPushedPulled = resource
273+
this.lastPushedPulled = resource
266274
}
267275
}
268276

@@ -332,13 +340,13 @@ open class EditorResource(
332340
}
333341

334342
fun dispose() {
335-
if (disposed) {
343+
if (disposed.get()) {
336344
return
337345
}
338-
this.disposed = true
339-
clusterResource?.close()
346+
this.disposed.set(true)
340347
setLastPushedPulled(null)
341348
setResourceVersion(null)
349+
clusterResource?.close()
342350
}
343351

344352
private fun createClusterResource(resource: HasMetadata): ClusterResource? {

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

+14
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,20 @@ class Error(val title: String, val message: String? = null): EditorResourceState
6363
}
6464
}
6565

66+
class Disposed: EditorResourceState() {
67+
68+
override fun equals(other: Any?): Boolean {
69+
if (this === other) {
70+
return true
71+
}
72+
return other is Disposed
73+
}
74+
75+
override fun hashCode(): Int {
76+
return Objects.hash()
77+
}
78+
}
79+
6680
open class Identical: EditorResourceState()
6781

6882
abstract class Different(val exists: Boolean, val isOutdatedVersion: Boolean): EditorResourceState() {

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

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

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

+46
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,8 @@ class EditorResourceStateTest {
114114
assertThat(Error("yoda", "is a green gnome")).isNotEqualTo(Error("yoda", "is a jedi"))
115115
assertThat(Error("obiwan", "is a jedi")).isNotEqualTo(Error("yoda", "is a jedi"))
116116

117+
assertThat(Error("obiwan", "is a jedi")).isNotEqualTo(Disposed())
118+
117119
assertThat(Error("yoda", "is a jedi")).isNotEqualTo(Identical())
118120

119121
assertThat(Error("yoda", "is a jedi")).isNotEqualTo(Modified(true, true))
@@ -133,12 +135,41 @@ class EditorResourceStateTest {
133135
assertThat(Error("yoda", "is a jedi")).isNotEqualTo(Pulled())
134136
}
135137

138+
@Test
139+
fun `Disposed is not equal to all the other states`() {
140+
// given
141+
// when
142+
assertThat(Disposed()).isNotEqualTo(Error("yoda", "is a jedi"))
143+
144+
assertThat(Disposed()).isEqualTo(Disposed())
145+
146+
assertThat(Disposed()).isNotEqualTo(Identical())
147+
148+
assertThat(Disposed()).isNotEqualTo(Modified(true, true))
149+
assertThat(Disposed()).isNotEqualTo(Modified(true, false))
150+
assertThat(Disposed()).isNotEqualTo(Modified(false, false))
151+
152+
assertThat(Disposed()).isNotEqualTo(DeletedOnCluster())
153+
154+
assertThat(Disposed()).isNotEqualTo(Outdated())
155+
156+
assertThat(Disposed()).isNotEqualTo(Created(true))
157+
assertThat(Disposed()).isNotEqualTo(Created(false))
158+
159+
assertThat(Disposed()).isNotEqualTo(Updated(true))
160+
assertThat(Disposed()).isNotEqualTo(Updated(false))
161+
162+
assertThat(Disposed()).isNotEqualTo(Pulled())
163+
}
164+
136165
@Test
137166
fun `Identical is not equal to all the other states`() {
138167
// given
139168
// when
140169
assertThat(Identical()).isNotEqualTo(Error("yoda", "is a jedi"))
141170

171+
assertThat(Identical()).isNotEqualTo(Disposed())
172+
142173
assertThat(Identical()).isEqualTo(Identical())
143174

144175
assertThat(Identical()).isNotEqualTo(Modified(true, true))
@@ -167,6 +198,10 @@ class EditorResourceStateTest {
167198
assertThat(Modified(false, false)).isNotEqualTo(Error("yoda", "is a jedi"))
168199
assertThat(Modified(false, true)).isNotEqualTo(Error("yoda", "is a jedi"))
169200

201+
assertThat(Modified(true, true)).isNotEqualTo(Disposed())
202+
assertThat(Modified(false, true)).isNotEqualTo(Disposed())
203+
assertThat(Modified(false, false)).isNotEqualTo(Disposed())
204+
170205
assertThat(Modified(true, true)).isNotEqualTo(Identical())
171206
assertThat(Modified(true, false)).isNotEqualTo(Identical())
172207
assertThat(Modified(false, false)).isNotEqualTo(Identical())
@@ -217,6 +252,8 @@ class EditorResourceStateTest {
217252
// when
218253
assertThat(DeletedOnCluster()).isNotEqualTo(Error("darth vader", "is on the dark side"))
219254

255+
assertThat(DeletedOnCluster()).isNotEqualTo(Disposed())
256+
220257
assertThat(DeletedOnCluster()).isNotEqualTo(Identical())
221258

222259
assertThat(DeletedOnCluster()).isNotEqualTo(Modified(true, true))
@@ -243,6 +280,8 @@ class EditorResourceStateTest {
243280
// when
244281
assertThat(Outdated()).isNotEqualTo(Error("obiwan", "is a jedi"))
245282

283+
assertThat(Outdated()).isNotEqualTo(Disposed())
284+
246285
assertThat(Outdated()).isNotEqualTo(Identical())
247286

248287
assertThat(Outdated()).isNotEqualTo(Modified(true, true))
@@ -270,6 +309,8 @@ class EditorResourceStateTest {
270309
assertThat(Created(true)).isNotEqualTo(Error("darth vader", "is on the dark side"))
271310
assertThat(Created(false)).isNotEqualTo(Error("darth vader", "is on the dark side"))
272311

312+
assertThat(Created(false)).isNotEqualTo(Disposed())
313+
273314
assertThat(Created(true)).isNotEqualTo(Identical())
274315
assertThat(Created(false)).isNotEqualTo(Identical())
275316

@@ -309,6 +350,9 @@ class EditorResourceStateTest {
309350
assertThat(Updated(true)).isNotEqualTo(Error("darth vader", "is on the dark side"))
310351
assertThat(Updated(false)).isNotEqualTo(Error("darth vader", "is on the dark side"))
311352

353+
assertThat(Updated(true)).isNotEqualTo(Disposed())
354+
assertThat(Updated(false)).isNotEqualTo(Disposed())
355+
312356
assertThat(Updated(true)).isNotEqualTo(Identical())
313357
assertThat(Updated(false)).isNotEqualTo(Identical())
314358

@@ -347,6 +391,8 @@ class EditorResourceStateTest {
347391
// when
348392
assertThat(Pulled()).isNotEqualTo(Error("darth vader", "is on the dark side"))
349393

394+
assertThat(Pulled()).isNotEqualTo(Disposed())
395+
350396
assertThat(Pulled()).isNotEqualTo(Identical())
351397

352398
assertThat(Pulled()).isNotEqualTo(Modified(true, true))

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

+23
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,29 @@ class EditorResourceTest {
513513
assertThat(state).isInstanceOf(Error::class.java)
514514
}
515515

516+
@Test
517+
fun `#getState should return Disposed if resource is disposed`() {
518+
// given
519+
val editorResource = createEditorResource(POD2)
520+
editorResource.disposed.set(true)
521+
val state = editorResource.getState()
522+
// then
523+
assertThat(state).isInstanceOf(Disposed::class.java)
524+
}
525+
526+
@Test
527+
fun `#getState should return Disposed even if different state was set after disposal`() {
528+
// given
529+
val editorResource = createEditorResource(POD2)
530+
editorResource.disposed.set(true)
531+
val shouldBeIgnored = mock<Error>()
532+
editorResource.setState(shouldBeIgnored)
533+
// when
534+
val state = editorResource.getState()
535+
// then
536+
assertThat(state).isInstanceOf(Disposed::class.java)
537+
}
538+
516539
@Test
517540
fun `#dispose should close clusterResource that was created`() {
518541
// given

0 commit comments

Comments
 (0)