Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Workaround for KT-58685 #3881

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Workaround for KT-58685 #3881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14ea7af
Test case to reproduce KT-58685
CLOVIS-AI Sep 6, 2023
9c42130
Workaround for KT-58685
CLOVIS-AI Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions kotlinx-coroutines-core/common/src/sync/Mutex.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,18 @@ public suspend inline fun <T> Mutex.withLock(owner: Any? = null, action: () -> T
callsInPlace(action, InvocationKind.EXACTLY_ONCE)
}

// Cannot use 'finally' in this function because of KT-58685
// See kotlinx.coroutines.sync.MutexTest.testWithLockJsMiscompilation

lock(owner)
try {
return action()
} finally {
val result = try {
action()
} catch (e: Throwable) {
unlock(owner)
throw e
}
Comment on lines +131 to 134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To recreate the behavior of finally, if unlock inside catch throws an exception, this exception must add e as a suppressed one. Could you also write the corresponding test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sorry for the time it took me to get back to this.

I'm not sure exactly what you mean here, something like this?

try {
    action()
} catch (e: Throwable) {
    try {
        unlock(owner)
    } catch (suppressed: Throwable) {
        e.addSuppressed(suppressed)
    }
    throw e
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I was mistaken. There's no such behavior currently, and on second thought, I don't think there should be.

unlock(owner)
return result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you'll agree that this is an unnatural way to write this. You shouldn't trust me to remember not to "fix" the style of code accidentally unless you drop a comment!

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

@CLOVIS-AI CLOVIS-AI Sep 27, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment mentioning the issue and linking to the test case.

}


Expand Down
12 changes: 9 additions & 3 deletions kotlinx-coroutines-core/common/src/sync/Semaphore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,18 @@ public suspend inline fun <T> Semaphore.withPermit(action: () -> T): T {
callsInPlace(action, InvocationKind.EXACTLY_ONCE)
}

// Cannot use 'finally' in this function because of KT-58685
// See kotlinx.coroutines.sync.SemaphoreTest.testWithPermitJsMiscompilation

acquire()
try {
return action()
} finally {
val result = try {
action()
} catch (e: Throwable) {
release()
throw e
}
release()
return result
}

@Suppress("UNCHECKED_CAST")
Expand Down
16 changes: 16 additions & 0 deletions kotlinx-coroutines-core/common/test/sync/MutexTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,4 +148,20 @@ class MutexTest : TestBase() {
assertFailsWith<IllegalStateException> { mutex.lock(owner) }
assertFailsWith<IllegalStateException> { select { mutex.onLock(owner) {} } }
}

@Test
fun testWithLockJsMiscompilation() = runTest {
// This is a reproducer for KT-58685
// On Kotlin/JS IR, the compiler miscompiles calls to 'unlock' in an inlined finally
// This is visible on the withLock function
// Until the compiler bug is fixed, this test case checks that we do not suffer from it
val mutex = Mutex()
assertFailsWith<IndexOutOfBoundsException> {
try {
mutex.withLock { null } ?: throw IndexOutOfBoundsException() // should throw…
} catch (e: Exception) {
throw e // …but instead fails here
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a complementary test, like testJsMiscompilationIsStillHappening, that would check that with an alternative, naturally-written definition of withLock, anomalous behavior does happen. This way, when the bad behavior stops, the test will fail and we'll know that the workaround is no longer needed. This will also ensure that testWithLockJsMiscompilation doesn't just work accidentally, we'll have a clear distinction between "this version clearly works" and "that version clearly doesn't." Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add this, won't it means that whenever it is fixed on the compiler side, it will break CI until someone rolls back this PR?

I'll try to do this using the reproducer from the issue.

Copy link
Contributor Author

@CLOVIS-AI CLOVIS-AI Sep 27, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, thinking back on this, I'm not sure it's so easy. This function is inline, the bug happens at the call site.

Therefore, even if the KotlinX.Coroutines team uses a compiler that doesn't have the issue anymore, a user of the library could still be using an older compiler that has the issue, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the point about CI makes sense, given that the issue is already fixed on the compiler's side, so we don't need a notification.

Regarding the old compiler + new library version: we don't provide guarantees as to what happens when using a library compiled with a newer compiler than your code uses. Generally, it's error-prone.

So, let's not add the tests. I wrote one just to make sure the tests that you added do correctly show that there's no problem, and sure enough, your tests do invoke the miscompilation if we keep the old form of withLock/withPermit. Nice work!

class SemaphoreTest: TestBase() {
    @Test
    fun testWithPermitJsMiscompilation() = runTest {
        // This is a reproducer for KT-58685
        // On Kotlin/JS IR, the compiler miscompiles calls to 'release' in an inlined finally
        // This is visible on the withPermit function
        // Until the compiler bug is fixed, this test case checks that we do suffer from it
        val semaphore = Semaphore(1)
        assertFailsWith<IllegalStateException> {
            try {
                semaphore.withPermitMiscompiled { null } ?: throw IndexOutOfBoundsException() // should throw…
            } catch (e: Exception) {
                throw e // …but instead fails here
            }
        }
    }
}

suspend inline fun <T> Semaphore.withPermitMiscompiled(action: () -> T): T {
    acquire()
    try {
        return action()
    } finally {
        release()
    }
}

}
18 changes: 17 additions & 1 deletion kotlinx-coroutines-core/common/test/sync/SemaphoreTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,20 @@ class SemaphoreTest : TestBase() {
assertFailsWith<IllegalArgumentException> { Semaphore(1, -1) }
assertFailsWith<IllegalArgumentException> { Semaphore(1, 2) }
}
}

@Test
fun testWithPermitJsMiscompilation() = runTest {
// This is a reproducer for KT-58685
// On Kotlin/JS IR, the compiler miscompiles calls to 'release' in an inlined finally
// This is visible on the withPermit function
// Until the compiler bug is fixed, this test case checks that we do not suffer from it
val semaphore = Semaphore(1)
assertFailsWith<IndexOutOfBoundsException> {
try {
semaphore.withPermit { null } ?: throw IndexOutOfBoundsException() // should throw…
} catch (e: Exception) {
throw e // …but instead fails here
}
}
}
}