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.

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

Workaround for KT-58685 #3881

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

CLOVIS-AI
Copy link
Contributor

Since at least Kotlin 1.8.0, the Kotlin/JS IR compiler miscompiles a specific combination of a caller of an inline function containing a finally block: the finally block is called twice. This is rare, but can happen when a user calls Mutex.withLock or Semaphore.withPermit.

I originally reported this issue to the library (#3754) without any idea how to reproduce it. @dkhalanskyjb managed to create a minimal reproducer and created a ticket against the Kotlin compiler (KT-58685). After discussing on the Kotlin Slack (thread), @rjaros found the workaround contained in this PR. Thanks to @JSMonk as well for attempting to fix this on the compiler side.

I added a unit test that reproduces the bug on the JS platform for both Mutex.withLock and Semaphore.withPermit. I then applied the workaround to both sources, ensuring it fixes both tests. Until a proper fix is found for the compiler, this should ensure users of the library do not have to deal with this. I believe the workaround has no impact on the library, though of course I'm open to your feedback.

For a more real-life example, this has been blocking the release of the Pedestal library for a long time now (#101) because it appears in our cache implementation.

}
unlock(owner)
return result
Copy link
Collaborator

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.

Copy link
Contributor Author

@CLOVIS-AI CLOVIS-AI Sep 27, 2023

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.

throw e // …but instead fails here
}
}
}
Copy link
Collaborator

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

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
Collaborator

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()
    }
}

@CLOVIS-AI
Copy link
Contributor Author

Bump @dkhalanskyjb, in case my previous messages got lost somewhere (if they didn't, and you're just busy at the moment, sorry for the ping)

Comment on lines +131 to 134
} catch (e: Throwable) {
unlock(owner)
throw e
}
Copy link
Collaborator

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
Collaborator

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.

throw e // …but instead fails here
}
}
}
Copy link
Collaborator

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()
    }
}

@dkhalanskyjb dkhalanskyjb self-requested a review November 2, 2023 08:38
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

Good job, thank you!

@dkhalanskyjb dkhalanskyjb merged commit 5a570e1 into Kotlin:develop Nov 2, 2023
@CLOVIS-AI CLOVIS-AI deleted the js-finally-miscompilation branch November 2, 2023 08:43
@lppedd
Copy link

lppedd commented Nov 13, 2023

Haven't followed the entire discussion, but since KT-58685 is now fixed, and will probably be available in 1.9.21, what's the purpose of this workaround? To keep it working in previous releases?

@dkhalanskyjb
Copy link
Collaborator

Yes, for a while. In practice, some people use the newer versions of the library without upgrading the compiler.

@Amejonah1200
Copy link

This "workaround" breaks non-local returns, as reported in #3985 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants