From 893381b05fbd55eef74dbfc663dd27cceb098901 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 29 Feb 2024 22:22:46 -0500 Subject: [PATCH 1/3] create test to prove getService is not thread safe Create test to confirm that ServiceBuilder.getService() should return the same instance, even when accessed from two threads. To ensure the test is consistent we made the constructor slow so the thread both end on the same line of code. --- .../onesignal/common/ServiceProviderTest.kt | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ServiceProviderTest.kt diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ServiceProviderTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ServiceProviderTest.kt new file mode 100644 index 0000000000..4738d513ae --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ServiceProviderTest.kt @@ -0,0 +1,44 @@ +package com.onesignal.common + +import com.onesignal.common.services.ServiceBuilder +import com.onesignal.common.services.ServiceProvider +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.types.shouldBeSameInstanceAs +import java.util.concurrent.LinkedBlockingQueue + +internal interface IMyTestInterface + +internal class MySlowConstructorClass : IMyTestInterface { + init { + // NOTE: Keep these println calls, otherwise Kotlin optimizes + // something which cases the test not fail when it should. + println("MySlowConstructorClass BEFORE") + Thread.sleep(10) + println("MySlowConstructorClass AFTER") + } +} + +class ServiceProviderTest : FunSpec({ + + fun setupServiceProviderWithSlowInitClass(): ServiceProvider { + val serviceBuilder = ServiceBuilder() + serviceBuilder.register().provides() + return serviceBuilder.build() + } + + test("getService is thread safe") { + val services = setupServiceProviderWithSlowInitClass() + + val queue = LinkedBlockingQueue() + Thread { + queue.add(services.getService()) + }.start() + Thread { + queue.add(services.getService()) + }.start() + + val firstReference = queue.take() + val secondReference = queue.take() + firstReference shouldBeSameInstanceAs secondReference + } +}) From 0e613e39b2be921c1453f5145c9b13635df3427e Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 29 Feb 2024 22:49:39 -0500 Subject: [PATCH 2/3] make ServiceProvider's serviceMap thread safe Add synchronized around all access to ServiceProvider's serviceMap. This ensures nothing can modify this map, resulting in multiple instance of a class being return unexpectedly. Another critical path is the call to resolve(this), where this logic could also take enough time that could case threading issues that is now wrapped in this new lock. This most likely will fix the real world problem we have been seeing where IApplicationService.appContext is null unexpectedly. One discovered path is ReceiveReceiptWorker is created from the Android OS on a background thread and it calls getService() without first calling OneSignal.initWithContext, resulting in a race condition where two instance of IApplicationService can be created. ReceiveReceiptWorker probably should call OneSignal.initWithContext first, but changing this is out of scope for this commit. --- .../common/services/ServiceProvider.kt | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt index c2630baa39..d1b7acc1eb 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt @@ -9,6 +9,7 @@ class ServiceProvider( registrations: List>, ) : IServiceProvider { private var serviceMap: Map, List>> + private val serviceMapLock = Any() init { val serviceMap = mutableMapOf, MutableList>>() @@ -44,23 +45,27 @@ class ServiceProvider( } override fun hasService(c: Class): Boolean { - return serviceMap.containsKey(c) + synchronized(serviceMapLock) { + return serviceMap.containsKey(c) + } } override fun getAllServices(c: Class): List { - val listOfServices: MutableList = mutableListOf() + synchronized(serviceMapLock) { + val listOfServices: MutableList = mutableListOf() - if (serviceMap.containsKey(c)) { - for (serviceReg in serviceMap!![c]!!) { - val service = - serviceReg.resolve(this) as T? - ?: throw Exception("Could not instantiate service: $serviceReg") + if (serviceMap.containsKey(c)) { + for (serviceReg in serviceMap!![c]!!) { + val service = + serviceReg.resolve(this) as T? + ?: throw Exception("Could not instantiate service: $serviceReg") - listOfServices.add(service) + listOfServices.add(service) + } } - } - return listOfServices + return listOfServices + } } override fun getService(c: Class): T { @@ -74,11 +79,10 @@ class ServiceProvider( } override fun getServiceOrNull(c: Class): T? { - Logging.debug("${indent}Retrieving service $c") -// indent += " " - val service = serviceMap[c]?.last()?.resolve(this) as T? -// indent = indent.substring(0, indent.length-2) - return service + synchronized(serviceMapLock) { + Logging.debug("${indent}Retrieving service $c") + return serviceMap[c]?.last()?.resolve(this) as T? + } } companion object { From 3932338b9f82f0f673923296a7c2085b15dcf5a6 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 29 Feb 2024 23:47:06 -0500 Subject: [PATCH 3/3] simplify ServiceProvider's serviceMap and lock No logic changes in this commit --- .../onesignal/common/services/ServiceProvider.kt | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt index d1b7acc1eb..aaa767c3ab 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt @@ -8,12 +8,9 @@ import com.onesignal.debug.internal.logging.Logging class ServiceProvider( registrations: List>, ) : IServiceProvider { - private var serviceMap: Map, List>> - private val serviceMapLock = Any() + private val serviceMap = mutableMapOf, MutableList>>() init { - val serviceMap = mutableMapOf, MutableList>>() - // go through the registrations to create the service map for easier lookup post-build for (reg in registrations) { for (service in reg.services) { @@ -24,8 +21,6 @@ class ServiceProvider( } } } - - this.serviceMap = serviceMap } internal inline fun hasService(): Boolean { @@ -45,13 +40,13 @@ class ServiceProvider( } override fun hasService(c: Class): Boolean { - synchronized(serviceMapLock) { + synchronized(serviceMap) { return serviceMap.containsKey(c) } } override fun getAllServices(c: Class): List { - synchronized(serviceMapLock) { + synchronized(serviceMap) { val listOfServices: MutableList = mutableListOf() if (serviceMap.containsKey(c)) { @@ -79,7 +74,7 @@ class ServiceProvider( } override fun getServiceOrNull(c: Class): T? { - synchronized(serviceMapLock) { + synchronized(serviceMap) { Logging.debug("${indent}Retrieving service $c") return serviceMap[c]?.last()?.resolve(this) as T? }