Mark classes as "Not Thread Safe"

Let’s assume we have a class with a mutable field. This class has a method which uses the field:

class MyClass {
    var owner: Person? = null

    fun getOwnerName(): String {
        if(this.owner == null){
            return "Nobody"
        }
        return "${this.owner.firstName} ${this.owner.lastName}" 
    }
}

The last line in ownerName() will not compile. The compiler will complain that this.owner is a mutable nullable field, and even though we checked that this.owner != null, it can’t let us access owner.firstName or owner.lastName without using !! because the property value might have been changed by a different thread.

Now, one could re-design the class to be more functional-style, or simply don’t bother at all and slap the !! in there. However, I have a different proposal:

@NotThreadSafe
class MyClass { /* content stays the same */ }

This would have two effects:

  • It would instruct the Kotlin compiler that it doesn’t need to consider concurrent changes when checking field nullness (so our getOwnerName() method would compile without error).

  • In addition, this annotation would tell any programmer using this class “hey, be careful when you use this class in multi-threaded environments”.

Any thoughts?

1 Like

You have to consider non trivial cases.

@NotThreadSafe
class MyClass {
    var owner: Person? = null

    fun getOwnerName(): String {
        if (this.owner == null) {
            return "Nobody"
        }
        consumeOwner()
        return "${this.owner.firstName} ${this.owner.lastName}"
    }

    private fun consumeOwner() {
        owner = null
    }
}

Yes, that’s a good point. However, as long as the field in question is private, the compiler can keep track of which method potentially modifies which field. For a first shot, the situation would improve if we impose the restriction that no method calls on this happen between the null-check and the field access.

EDIT: but then again, this can be hidden in a variable, or escape as an argument to a function… this would be very tricky.

EDIT2: To rectify the situation, we would require a constraint: no impure methods must be called between the null-check and the field access. However, the Kotlin compiler (as far as I know) has no notion of “purity” until now.

Why not store the field locally

fun getOwnerName(): String {
    val owner = this.owner?: return "Nobody"
    return "${owner.firstName} ${owner.lastName}" 
}

Or using let?

fun getOwnerName(): String {
    return owner
        ?.let{ "${it.firstName} ${it.lastName}" }
        ?: "NOBODY
}

Just curious. I think you have a good reason…

The primary reason is that it adds extra code, but no additional information. It’s noise. My primary issue with the additional local variable is that this compiles (in contrast to the code in the opening post):

fun getOwnerName(): String {
    val owner = this.owner // noise to satisfy the compiler
    if(owner == null) { return "Nobody" }
    return "${owner.firstName} ${owner.lastName}"
}

I think it is a legitimate question to ask how this code is “better” than the shorter version which Kotlin refuses to compile. Indeed, this code will never throw a NullPointerException. However, that doesn’t make it any more or less thread safe (this is the reason the Kotlin compiler gives for not accepting the null check on the mutable owner property). So this code has additional syntax, but all it accomplishes is that it trades a potential NullPointerException with a potential wrong result of getOwnerName. This class should never be used concurrently without further locking, no matter how we look at it. As part of a single thread, however, the null-check on the mutable property should be perfectly fine, yet Kotlin claims it isn’t.

Don’t get me wrong: I can see why Kotlin does this, the intention is plain and clear. But:

a) The error message it gives sounds like a very poor excuse. As noted by @fvasco above, the real reason is likely that Kotlin can’t track property modifications across method calls. Such data flow analysis is extremely performance- and memory-intensive, so I get why kotlinc doesn’t do it - it’s just too much for a compiler to handle. It’s just the argumentation based on concurrency in the error message that bothers me.

b) If you fix it with a local variable, a teammate may be inclined to come along, see it and may be tempted to remove it. Then the compiler complains and I (as the Kotlin proponent in our team) get an annoyed “Oh Kotlin is so weird” again. This case in particular (as I said, even though it is clear to me why this happens) is hard to grasp for programmers who are used to non-null-safe languages. I had to spend a good 5 minutes of contemplation to figure out what the problem is when I first encountered it myself.

I think that the functions that store field in variables at the beginning of a method are a little bit more thread aware than functions that use field and assume they don’t change.
I do however agree that both are not completely thread safe.

I also agree that the first case looks like boilerplate.
The second case looks pretty clear to me, although I agree that the decompiled code seems less obvious.
I believe however that if you want to use Java, you don’t have the problem at all.

At the moment I would choose a third option, which is very clear, but only works in easy cases like this, so isn’t really the solution:

fun Person.fullName() = "$firstName $lastName"
fun getOwnerName() = owner?.fullName() ?: "NOBODY"

Ps. Take a look at this topic:

Where I want to highlight:
https://youtrack.jetbrains.com/issue/KT-20294

In addition to all this, it is much harder for the jit to optimize field access versus local variable access. With a local the access is guaranteed to be local only. Fields need to guard against external access from any possible thread. While it is technical to do whole-program analysis to determine that does not happen it is much harder than needed (and needs to be redone, perhaps incrementally, when a new class is loaded - this happens quite often as the JVM loads classes lazily on first use)