From ef177e7a04c60a1f0e71fe26ae2879ba22afa087 Mon Sep 17 00:00:00 2001 From: Eldar Abusalimov Date: Wed, 6 Nov 2019 04:24:19 +0300 Subject: [PATCH] Expiration: Fix race with GC when handling already disposed Disposable On a rare occasion an instance of DisposableExpiration created for an already disposed Disposable could have been left non-expired. This used to happen when the GC had already collected a weakly-referenced child disposable by the time we disposed the instance of eakReferenceDisposableWrapper referencing the child. This also fixes NonBlockingReadActionTest.testSyncExecutionThrowsPCEWhenExpired failing randomly. GitOrigin-RevId: 28dfeef0e7d1ef501db0554a5d8069f82603211d --- .../openapi/application/constraints/Expiration.kt | 2 ++ .../openapi/application/constraints/ExpirationTest.kt | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/platform/platform-impl/src/com/intellij/openapi/application/constraints/Expiration.kt b/platform/platform-impl/src/com/intellij/openapi/application/constraints/Expiration.kt index aeb64f46cfc5..f4702522c8ae 100644 --- a/platform/platform-impl/src/com/intellij/openapi/application/constraints/Expiration.kt +++ b/platform/platform-impl/src/com/intellij/openapi/application/constraints/Expiration.kt @@ -5,6 +5,7 @@ import com.intellij.openapi.Disposable import com.intellij.openapi.WeakReferenceDisposableWrapper import com.intellij.openapi.util.Disposer import com.intellij.util.IncorrectOperationException +import com.intellij.util.ObjectUtils import kotlinx.coroutines.* /** @@ -146,6 +147,7 @@ internal fun Disposable.cancelJobOnDisposal(job: Job, if (!tryRegisterDisposable(this, childRef)) { Disposer.dispose(childRef) // runs disposableBlock() + ObjectUtils.reachabilityFence(child) return AutoCloseable { } } else { diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/application/constraints/ExpirationTest.kt b/platform/platform-tests/testSrc/com/intellij/openapi/application/constraints/ExpirationTest.kt index a3e620865063..674af5784e84 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/application/constraints/ExpirationTest.kt +++ b/platform/platform-tests/testSrc/com/intellij/openapi/application/constraints/ExpirationTest.kt @@ -12,7 +12,7 @@ import kotlinx.coroutines.Job class ExpirationTest : LightPlatformTestCase() { private fun createTestDisposable(): Disposable { - return Disposable { }.also { Disposer.register(testRootDisposable, it) } + return Disposer.newDisposable(name).also { Disposer.register(testRootDisposable, it) } } private fun createCompositeExpiration(vararg expiration: Expiration): Expiration { @@ -20,6 +20,13 @@ class ExpirationTest : LightPlatformTestCase() { return Expiration.composeExpiration(expiration.toList())!! } + fun `test isExpired for already disposed Disposable`() { + val disposable = createTestDisposable() + Disposer.dispose(disposable) + val expiration = DisposableExpiration(disposable) + assertTrue("Expiration.isExpired must be true", expiration.isExpired) + } + fun `test isExpired for JobExpiration`() { val job = Job() val expiration = JobExpiration(job)