Strange kotlin checkNotNullParameter error

user_2139586 0
Viewed 136

we received a crash on Firebase for a kotlin method:

Fatal Exception: java.lang.NullPointerException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkNotNullParameter, parameter code
       at [redacted].DeliveryMethod.<init>(:2)
       at [redacted].DeliveryMethodsUpdater$addSingleDMInAd$clientCall$1.invokeSuspend(DeliveryMethodsUpdater.kt:121)
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
       at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:738)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

the model is this one:

class DeliveryMethod() {
    lateinit var code: String
    lateinit var name: String
    lateinit var description: String

var isAddressRequired: Boolean? = null
var image: JSONObject? = null
var isDefault: Boolean = false

constructor(code: String) : this() {
    this.code = code
}

constructor(code: String, name: String, description: String, image: JSONObject? = null) : this() {
    this.code = code
    this.name = name
    this.description = description
    this.image = image
}
}

and the method:

private suspend fun addSingleDMInAd(
        adId: Long,
        deliveryMethodCode: String
    ): JoinAdDeliveryMethod? {
        var addedDeliveryMethod: JoinAdDeliveryMethod? = null
        val clientCall = GlobalScope.async(Dispatchers.IO) {
            val cd = CountDownLatch(1)
            Client.getInstance().addDeliveryMethodInAd(
                adId,
                DeliveryMethod(deliveryMethodCode),
                object : NetworkCallback<JoinAdDeliveryMethod> {
                    override fun onSuccess(result: JoinAdDeliveryMethod) {
                        addedDeliveryMethod = result
                        cd.countDown()
                    }

                    override fun onFailure(err: NetworkError?) {
                        addedDeliveryMethod = null
                        cd.countDown()
                    }
                }
            )
            cd.await()
        }
        clientCall.await()
        return addedDeliveryMethod
    }

now, I understand that that the constructor for DeliveryMethod is being called with a null value for code, but I don't understand why the exception only come up at this point. As you can see, the method param is also marked as non-null, and so are previous methods. Shouldn't the exception be thrown way before getting to the constructor call for DeliveryMethod?

EDIT: this is the caller of addSingleDMinAd():

 fun addDeliveryMethodsInAd(
        adId: Long,
        deliveryMethodCodesToAdd: List<String>,
        completionListener: (List<JoinAdDeliveryMethod?>) -> Unit
    ) {
        GlobalScope.launch {
            val updatedDms: MutableList<JoinAdDeliveryMethod?> = mutableListOf()
            for (deliveryCode in deliveryMethodCodesToAdd) {
                addSingleDMInAd(adId = adId, deliveryMethodCode = deliveryCode).run {
                    updatedDms.add(this)
                }
            }
            completionListener.invoke(updatedDms)
        }
    }

this is the java caller of the addDeliveryMethodsInAd() (this is inside an Android Service):

new DeliveryMethodsUpdater().addDeliveryMethodsInAd(
                            result.getId(),
                            deliveryMethodCodesToAdd,
                            updatedDMs -> {
                                // on failed delivery method request
                                for (JoinAdDeliveryMethod updatedDm : updatedDMs) {
                                    if (updatedDm == null) {
                                        //show error
                                        break;
                                    }
                                }
                                AdDetailUpdater
                                        .getInstance()
                                        .updateSubscribersWithDeliveryMethods(result.getId(), updatedDMs);
                                return null;
                            }
                    );
kotlin
1 Answers

Shouldn't the exception be thrown way before getting to the constructor call for DeliveryMethod?

Within Kotlin, it's not possible for a non-null parameter to be given a null value at runtime accidentally (because the code wouldn't have compiled in the first place). However, this can happen if the value is passed from Java. This is why the Kotlin compiler tries to protect you from Java's null unsafety by generating null-checks at the beginning of some methods (with the intrinsic checkNotNullParameter you're seeing fail here).

However, there is no point in doing that in private or suspend methods since they can only be called from Kotlin (usually), and it would add some overhead that might not be acceptable in performance-sensitive code. That is why these checks are only generated for non-suspend public/protected/internal methods (because their goal is to prevent misuse from Java).

This is why, if you manage to call addSingleDMInAd with a null argument, it doesn't fail with this error. That said, it would be interesting to see how you're getting the null here, because usually the checks at the public API surface are enough. Is some reflection or unsafe cast involved here?

EDIT: with the addition of the calling code, this clears up the problem. You're calling a method that takes a List<String> from Java, with a list that contains nulls. Unfortunately Kotlin only checks the parameters themselves (in this case, it checks that the list itself is not null), it doesn't iterate your list to check for nulls inside. This is why it didn't fail at the public API surface in this case.

Also, the way your model is setup is quite strange. It seems the lateinit is lying because depending on which constructor is used, the properties may actually not be set at all. It would be safer to mark them as nullable to account for when users of that class don't set the value of these properties. Doing this, you won't even need all secondary constructors, and you can just use default values:

class DeliveryMethod() {
    var code: String? = null,
    var name: String? = null,
    var description: String? = null,
    var image: JSONObject? = null,
) {
    var isAddressRequired: Boolean? = null
    var isDefault: Boolean = false
}

Other things of note about addSingleDMInAd:

  • don't use GlobalScope in this case. If you need to run short-lived coroutines, provide them with a smaller scope that is cancelled when the work is not needed anymore - it ensures no coroutines are leaked. You can read more about the potential pitfalls of GlobalScope and possible replacements in its own doc. That said, you probably shouldn't be starting a coroutine at all anyway here, see next point.

  • don't use async {} if you use await() right after - it's pointless to start something asynchronous if you wait for it right there. If you want to switch the context to IO, use withContext(Dispatchers.IO) { ... } instead. That said, you shouldn't even need to use the IO dispatcher here, see next point.

  • don't use CountDownLatch for this purpose. The proper way to encapsulate an async API as a suspend function for coroutines is to use suspendCancellableCoroutine (check its doc, it provides an example on how to use it). Once you use this, there is no need anymore for Dispatchers.IO because it will not be blocking the current thread anymore.

user_15408180
Related