Hi, it's now been almost six months that we cannot use locks on Kotlin/JS because the compiler tries...

# javascript

[CLOVIS]

Hi, it's now been almost six months that we cannot use locks on Kotlin/JS because the compiler tries to unlock them twice. Adopting Kotlin/JS was indeed a risk that we decided to take, but it's the first time I ever experienced such a roadblock. For the past three years or so since I started using Kotlin/JS in production, I've never faced any blocking issues. If anyone knows how to get locks to behave normally (even if it's just a workaround), please let me know. If not, I'd appreciate a +1 on the issue so the team will see it. YouTrack issue

[Robert Jaros]

This version of withLock function seems to work correctly, and (I think) should work exactly the same:

private suspend inline fun <T> withLock(block: () -> T): T {
    lock()
    val result = try {
        block()
    } catch (e: Throwable) {
        unlock()
        throw e
    }
    unlock()
    return result
}

[CLOVIS]

Wasn't KotlinX.Coroutines available on the Kotlin Playground? I was trying to check your variant, but the reproducer doesn't seem to compile

[Artem Kobzar]

@CLOVIS we are so sorry for your experience. I will take a look on the problem right now and will send you the investigation information as soon as I find something.

[Artem Kobzar]

Btw, what is the compiler version you use?

[CLOVIS]

I originally noticed it on 1.8.20, it was still an issue in 1.8.21.

[CLOVIS]

I originally reported it to the KotlinX.Coroutines team, the YouTrack ticket above was created by them because it wasn't on their side. You can find my full reproducer in the original ticket, but it's buried very deep into the codebase. When they created the YouTrack ticket, Dmitry Khalanskiy added a simplified reproducer that doesn't depend on my code at all, hopefully this is enough information

[CLOVIS]

I will try @Robert Jaros's workaround later today and report back. If you need any more information, don't hesitate to ask (I will reply here faster than on YouTrack, but I do read both)

[CLOVIS]

@Robert Jaros I have tried your workaround, but the issue remains unchanged.

[Robert Jaros]

Which Kotlin version do you use in your project?

[CLOVIS]

1.8.20.

[CLOVIS]

We'll be in 1.9.0 in a few days, hopefully.

[CLOVIS]

We first noticed it 1.8.0 I think

[Robert Jaros]

I've tested this on 1.8.22 and 1.9.0 with the simplified reproducer code.

[CLOVIS]

I see the same result with 1.8.22. Same branch as my previous message.

[Robert Jaros]

I'll play a bit with pedestal. Perhaps I'll find something new :)

[CLOVIS]

To reproduce it using Pedesal:

git clone <https://gitlab.com/opensavvy/pedestal.git>
cd pedestal
git switch 101-lock-bug # ← that's where I put your reproducer, you can stay on the 'main' branch if you want to see it for real
./gradlew state:jsNodeTest

Tests opensavvy.state.FailureEndToEndTest.usersCannotGetUnsharedCounters and opensavvy.state.FailureEndToEndTest.usersCannotGetACounterThatDoesntExist will fail because of the bug.

[CLOVIS]

Version numbers are in versions.properties at the root of the repository (we're using #refreshversions)

[CLOVIS]

The main branch is going to change quite a bit very soon in order to use the version catalog, Kotlin 1.9.0 etc. AFAIK, it doesn't impact the problem.

[Robert Jaros]

Maybe I don't understand, but why have you replaced withLock to withLockHack only in one call?

[Robert Jaros]

If I replace all calls in the FailureEndToEndTest.kt the tests pass.

[CLOVIS]

Oh wow you're right. Sorry for the time lost.

[CLOVIS]

Thanks a lot, I'll use your workaround in the other places where it happens. Thanks a lot.

[CLOVIS]

Now I'm curious, what did you change exactly? Is it the inline ? Is it the finally ?

[Robert Jaros]

finally

[Robert Jaros]

I had issues with finally in Kotlin/JS earlier in some complex inline and suspending code. Unfortunately I've never managed to create a reproducer so I couldn't fill an issue.

[CLOVIS]

If I get a bit of free time, do you mind if I submit your workaround as a PR directly to kotlinx-coroutines? So it won't affect anyone else until the bug is fixed on the compiler side

[Robert Jaros]

I've just realized the original withLock is from kotlinx-coroutines :)

[CLOVIS]

yes, that's why I originally reported it to them. I think they had a bit of difficulty to provide a reproducer based on the massive project I gave them 😕

[Robert Jaros]

Please fill free to submit. I hope the compiler will also get a fix because it's definitely a serious compiler bug.

[CLOVIS]

I hope so as well, but I've almost been waiting 6 months at this point, so I'll accept any workaround without complaining 😅

[CLOVIS]

If you want me to credit you as co-author of the PR, please send me your GitHub email address (either here or in DM) 🙂

[CLOVIS]

I'll probably do it tomorrow night

[CLOVIS]

Oh, you're the developer of KVision, I didn't recognize you. I guess that explains why you'd spend time on Kotlin/JS haha

[Robert Jaros]

No need 🙂 You can take all the rewards 😉

[Robert Jaros]

Oh yes, I love to workaround Kotlin/JS bugs 😛

[CLOVIS]

Well, we did sign up for it. I much prefer when I find the workaround after a few hours than months though 😅

[Artem Kobzar]

I reproduced it yesterday in master and work on fix, that I hope, I will backport into 1.9.20

[CLOVIS]

@Artem Kobzar do you think it's worth submitting the workaround to Coroutines, then, or will 1.9.20 come before the next KotlinX version anyway?

[CLOVIS]

If you think it's worth it, I can submit it right now

[Artem Kobzar]

I've just prepared the fix for Kotlin master branch, and I think it will be better to make a workaround for kotlinx.coroutines until 1.9.20.
So, the problem appears with combination of inline functions and finally block inside of them.
The simplest workaround will be to remote inline keyword from the withLock function or to make some wrapper the function that will be a regular function (without inline keyword).

[CLOVIS]

What do you think of the workaround above in this thread? It requires neither removing inline nor creating a wrapper, and the duplication is minimal, so I feel it may be easier to integrate into KotlinX.Coroutines? I doubt removing inline would be accepted, since that's a binary-incompatible change.

[CLOVIS]

I created the PR: https://github.com/Kotlin/kotlinx.coroutines/pull/3881