Is Kotlin synchronized() not locking basic types?

user_5860352 0
Viewed 439
class Notification(val context: Context, title: String, message: String) {
    private val channelID = "TestMessages"

    companion object ID {
        var s_notificationID = -1
    }

    init {
        var notificationID = -1
        synchronized(s_notificationID) {
            if (++s_notificationID == 0)
                createNotificationChannel()
            notificationID = s_notificationID
        }

The above is being called simultaneously from two threads. A breakpoint in createNotificationChannel() clearly showed that sometimes s_notificationID equals 1.

However, if I change synchronized(s_notificationID) to synchronized(ID) then it seems to lock fine.

Is synchronized() not locking basic types? And if so, why does it compile?

kotlin
3 Answers

A look at the generated JVM bytecode indicates that the ID example looks like

synchronized(ID) { ... }

which is what you'd expect. However, the s_notificationID example looks more like

synchronized(Integer.valueOf(s_notificationID)) { ... }

In Java, we can only synchronize on objects, not on primitives. Kotlin mostly removes this distinction, but it looks like you've found one place where the implementation still seeps through. Since s_notificationID is an int as far as the JVM is concerned (hence, not an object) but synchronized expects an object, Kotlin is "smart" enough to wrap the value in Integer.valueOf on demand. Unfortunately for you, that produces wildly inconsistent results, because

This method will always cache values in the range -128 to 127, inclusive, and may cache other values outside of this range.

So for small numbers, this is guaranteed to lock on some cached object in memory that you don't control. For large ones, it may be a fresh object (hence always unlocked) or it might again end up on a cached object out of your hands.

The lesson here, it seems, is: Don't synchronize on primitive types.

user_22886590

Silvio Mayolo explained why it is not a good idea to synchronize on primitives (actually, I think the compiler should warn about this). But I believe there is another problem with this code, probably the main one that makes your synchronized blocks work in parallel.

The problem is that you replace the value of s_notificationID. Even if it would be an object, not a primitive, your synchronized blocks would still run in parallel, because each call to synchronized uses a different object. This is why in Java we usually synchronize on this and not on a field that we need to modify.

user_4488750

TL;DR The lesson here, it seems, is: Don't synchronize on primitive types.

synchronized(i) where i is Int, is actually synchronized(Integer.valueOf(i)). Only in the range -128 to 127 this value is guaranteed to be a cached value. Another fact is that ++i cannot be looked at as a mutation of the "object" i, but rather as replacing i by a new "object" with the value i+1.

Thank you broot & Silvio Mayolo for the above.

Experiments I did prove the above. In my original code I have removed the ++ from ++s_notificationID. Amazingly or not, the lock worked now. Now with that change I changed var s_notificationID = -1 to be var s_notificationID = -1000. Even more amazing, now the lock again stopped working.

Still, I think this anomaly of basic types undermines the attempt of Kotlin to see basic types as objects, and I think this should have been mentioned clearly in Kotlin documentation.

user_58603520
Related