Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(ts) Runtime de-globalling (step 3) #6758

Draft
wants to merge 32 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
3afe810
modernize dev-shell
Nov 6, 2020
a908f48
tweaks
Nov 6, 2020
a093036
law of Demeter
Nov 6, 2020
193c1a7
remove storageService
Nov 12, 2020
4dad548
cleanups
Nov 12, 2020
09fd6b6
surgically remove static (global) runtime object
Nov 17, 2020
32e20ae
clean ups
Nov 18, 2020
1987f50
big runtime usage refactor
Nov 20, 2020
16a0961
cleanups
Nov 20, 2020
ab675d4
cleanups
Nov 20, 2020
328ac92
serious degloballing
Nov 26, 2020
4978ea0
repairs after unpleasant rebase
Dec 8, 2020
1900fe2
remove debugger statement
Dec 8, 2020
00505f1
sleuthing test failures
Dec 8, 2020
6a9f14d
Don't call idle immediately after closing StorageEndpoint
jblebrun Dec 8, 2020
9482eef
Refactors several classes to use injected StorageKeyManager instances…
csilvestrini Dec 8, 2020
7f2ea17
rename EntitySpecTest.kt to javatests/arcs/sdk/GeneratedEntityTest.kt
galganif Dec 8, 2020
7f5e9f6
Fix typo
csilvestrini Dec 8, 2020
2aff690
Tests for BaseHandle.kt
Dec 8, 2020
bdfdce5
Add missing strict deps for type aliases.
arcs-c3po Dec 8, 2020
b4bb177
rename ReferenceSpecTest.kt to javatests/arcs/sdk/GeneratedReferenceT…
galganif Dec 8, 2020
7ebf1f0
add support for inline schema field inside ordered list in fromLitera…
mariakleiner Dec 8, 2020
b81fbd2
Rename EntityHandleManager to HandleManagerImpl to reflect the actual…
yuangu-google Dec 8, 2020
d5242e8
Add more unit tests for DirectStore.
yuangu-google Dec 8, 2020
f1092db
Caching RawEntity hashCode to improve overhead.
alxmrs Dec 9, 2020
408388c
Add error message for CollectionHandle init (#6690)
SHeimlich Dec 9, 2020
0557c0b
Refactor mock helper methods from the Handle tests into testutil/
Dec 9, 2020
be944aa
Remove unused methods from EntityHandleManager and StorageProxy.
Dec 9, 2020
0e2bf53
Create showcase & showcase test for queries
Dec 9, 2020
c9f4166
Disable import ordering in ktlint (#6749)
csilvestrini Dec 10, 2020
75c791e
remove dead code
Dec 10, 2020
4b6bc47
Merge branch 'master' of github.com:PolymerLabs/arcs into dec-dev-she…
Dec 10, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Refactors several classes to use injected StorageKeyManager instances…
… instead of accessing the StorageMeyManager.GLOBAL_INSTANCE directly.

I've only updated some really simple usages to use the DI pattern. There shouldn't be any change in behaviour; everything still points to the global singleton instance for now.

PiperOrigin-RevId: 346223334
  • Loading branch information
csilvestrini authored and Scott J. Miles committed Dec 10, 2020
commit 9482eef419ec5bcb27b88cc67cc99f8edf87fb27
3 changes: 2 additions & 1 deletion java/arcs/android/common/resurrection/DbHelper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import arcs.core.storage.api.DriverAndKeyConfigurator
@VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
class DbHelper(
context: Context,
private val storageKeyManager: StorageKeyManager,
dbName: String = RESURRECTION_DB_NAME
) : SQLiteOpenHelper(
context,
Expand Down Expand Up @@ -157,7 +158,7 @@ class DbHelper(
val notifiers =
notifiersByComponentName[requestedNotifier]
?: mutableListOf()
notifiers.add(StorageKeyManager.GLOBAL_INSTANCE.parse(key))
notifiers.add(storageKeyManager.parse(key))
notifiersByComponentName[requestedNotifier] = notifiers
}

Expand Down
14 changes: 10 additions & 4 deletions java/arcs/android/common/resurrection/ResurrectorService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import android.os.Bundle
import androidx.annotation.VisibleForTesting
import arcs.android.common.resurrection.ResurrectionRequest.UnregisterRequest
import arcs.core.storage.StorageKey
import arcs.core.storage.StorageKeyManager
import arcs.core.util.guardedBy
import java.io.PrintWriter
import kotlinx.coroutines.CoroutineName
Expand All @@ -42,13 +43,18 @@ abstract class ResurrectorService : Service() {
protected open val job =
Job() + Dispatchers.IO + CoroutineName("ResurrectorService")

private val dbHelper: DbHelper by lazy { DbHelper(this, resurrectionDatabaseName) }
// TODO(b/174432505): Don't use the GLOBAL_INSTANCE, accept as a constructor param instead.
protected val storageKeyManager = StorageKeyManager.GLOBAL_INSTANCE

private val dbHelper: DbHelper by lazy {
DbHelper(this, storageKeyManager, resurrectionDatabaseName)
}

private val mutex = Mutex()
private var registeredRequests: Set<ResurrectionRequest>
by guardedBy(mutex, setOf())
by guardedBy(mutex, setOf())
private var registeredRequestsByNotifiers: Map<StorageKey?, Set<ResurrectionRequest>>
by guardedBy(mutex, mapOf())
by guardedBy(mutex, mapOf())

@VisibleForTesting
var loadJob: Job? = null
Expand Down Expand Up @@ -118,7 +124,7 @@ abstract class ResurrectorService : Service() {
"""
Resurrection Requests
---------------------
""".trimIndent()
""".trimIndent()
)

val requests = StringBuilder().apply {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ package arcs.android.storage.database
import android.content.Context
import androidx.lifecycle.LifecycleObserver
import arcs.core.storage.StorageKey
import arcs.core.storage.StorageKeyManager
import arcs.core.storage.database.Database
import arcs.core.storage.database.DatabaseIdentifier
import arcs.core.storage.database.DatabaseManager
Expand All @@ -39,6 +40,9 @@ class AndroidSqliteDatabaseManager(
private val dbCache by guardedBy(mutex, mutableMapOf<DatabaseIdentifier, DatabaseImpl>())
override val registry = AndroidSqliteDatabaseRegistry(context)

// TODO(b/174432505): Don't use the GLOBAL_INSTANCE, accept as a constructor param instead.
private val storageKeyManager = StorageKeyManager.GLOBAL_INSTANCE

suspend fun close() {
mutex.withLock {
dbCache.values.forEach { it.close() }
Expand All @@ -52,7 +56,7 @@ class AndroidSqliteDatabaseManager(
val entry = registry.register(name, persistent)
return mutex.withLock {
dbCache[entry.name to entry.isPersistent]
?: DatabaseImpl(context, name, persistent) {
?: DatabaseImpl(context, storageKeyManager, name, persistent) {
mutex.withLock {
dbCache.remove(entry.name to entry.isPersistent)
}
Expand Down
9 changes: 5 additions & 4 deletions java/arcs/android/storage/database/DatabaseImpl.kt
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ typealias ReferenceId = Long
// Our helper extension methods close Cursors correctly.
class DatabaseImpl(
context: Context,
private val storageKeyManager: StorageKeyManager,
databaseName: String,
persistent: Boolean = true,
val onDatabaseClose: suspend () -> Unit = {}
Expand Down Expand Up @@ -435,7 +436,7 @@ class DatabaseImpl(
} else {
Reference(
id = it.getString(6),
storageKey = StorageKeyManager.GLOBAL_INSTANCE.parse(it.getString(7)),
storageKey = storageKeyManager.parse(it.getString(7)),
version = it.getVersionMap(8),
_creationTimestamp = it.getLong(9),
_expirationTimestamp = it.getLong(10),
Expand Down Expand Up @@ -1052,7 +1053,7 @@ class DatabaseImpl(
arrayOf()
).forEach {
val storageKeyId = it.getLong(0)
val storageKey = StorageKeyManager.GLOBAL_INSTANCE.parse(it.getString(1))
val storageKey = storageKeyManager.parse(it.getString(1))
val orphan = it.getNullableBoolean(2) ?: false
val noRef = it.getBoolean(3)
if (orphan && noRef) {
Expand Down Expand Up @@ -1379,7 +1380,7 @@ class DatabaseImpl(
)

(storageKeys union updatedContainersStorageKeys).map { storageKey ->
notifyClients(StorageKeyManager.GLOBAL_INSTANCE.parse(storageKey)) {
notifyClients(storageKeyManager.parse(storageKey)) {
it.onDatabaseDelete(null)
}
}
Expand Down Expand Up @@ -1720,7 +1721,7 @@ class DatabaseImpl(
ReferenceWithVersion(
Reference(
id = it.getString(0),
storageKey = StorageKeyManager.GLOBAL_INSTANCE.parse(it.getString(3)),
storageKey = storageKeyManager.parse(it.getString(3)),
version = it.getVersionMap(4),
_creationTimestamp = it.getLong(1),
_expirationTimestamp = it.getLong(2)
Expand Down
11 changes: 10 additions & 1 deletion java/arcs/core/host/AbstractArcHost.kt
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import arcs.core.host.api.HandleHolder
import arcs.core.host.api.Particle
import arcs.core.storage.StorageEndpointManager
import arcs.core.storage.StorageKey
import arcs.core.storage.StorageKeyManager
import arcs.core.util.LruCacheMap
import arcs.core.util.Scheduler
import arcs.core.util.TaggedLog
Expand Down Expand Up @@ -87,6 +88,9 @@ abstract class AbstractArcHost(
private val particleConstructors: MutableMap<ParticleIdentifier, ParticleConstructor> =
mutableMapOf()

// TODO(b/174432505): Don't use the GLOBAL_INSTANCE, accept as a constructor param instead.
private val storageKeyManager = StorageKeyManager.GLOBAL_INSTANCE

private val cacheMutex = Mutex()

/** In memory cache of [ArcHostContext] state. */
Expand Down Expand Up @@ -353,7 +357,12 @@ abstract class AbstractArcHost(
): ArcHostContextParticle {
val handleManager = entityHandleManager("$hostId-${arcHostContext.arcId}")

return ArcHostContextParticle(hostId, handleManager, this::instantiateParticle).apply {
return ArcHostContextParticle(
hostId,
handleManager,
storageKeyManager,
this::instantiateParticle
).apply {
val partition = createArcHostContextPersistencePlan(
arcHostContextCapability,
arcHostContext.arcId
Expand Down
3 changes: 2 additions & 1 deletion java/arcs/core/host/ArcHostContextParticle.kt
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ typealias ArcHostContextParticle_PlanHandle = AbstractArcHostContextParticle.Pla
class ArcHostContextParticle(
private val hostId: String,
private val handleManager: HandleManager,
private val storageKeyManager: StorageKeyManager,
private val instantiateParticle: suspend (ParticleIdentifier, Plan.Particle?) -> Particle,
private val instantiatedParticles: MutableMap<String, Particle> = mutableMapOf()
) : AbstractArcHostContextParticle() {
Expand Down Expand Up @@ -198,7 +199,7 @@ class ArcHostContextParticle(
}
handle.connectionName to Plan.HandleConnection(
Plan.Handle(
StorageKeyManager.GLOBAL_INSTANCE.parse(planHandle.storageKey),
storageKeyManager.parse(planHandle.storageKey),
// TODO(b/161818462): Properly serialize serialize Handle Type's schema.
fromTag(arcId, particle, planHandle.type, handle.connectionName),
emptyList()
Expand Down
3 changes: 2 additions & 1 deletion java/arcs/sdk/android/labs/host/ArcHostHelper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import kotlinx.coroutines.launch
*/
class ArcHostHelper(
private val service: Service,
private val storageKeyManager: StorageKeyManager,
vararg arcHosts: ArcHost
) {
private val job = SupervisorJob() + Dispatchers.Unconfined + CoroutineName("ArcHostHelper")
Expand Down Expand Up @@ -107,7 +108,7 @@ class ArcHostHelper(

arcHost.onResurrected(
targetId,
notifiers.map(StorageKeyManager.GLOBAL_INSTANCE::parse)
notifiers.map(storageKeyManager::parse)
)
}

Expand Down
6 changes: 5 additions & 1 deletion java/arcs/sdk/android/labs/host/ArcHostService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package arcs.sdk.android.labs.host
import android.content.Intent
import androidx.lifecycle.LifecycleService
import arcs.core.host.ArcHost
import arcs.core.storage.StorageKeyManager
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.MainScope
import kotlinx.coroutines.cancel
Expand All @@ -23,6 +24,9 @@ import kotlinx.coroutines.cancel
abstract class ArcHostService : LifecycleService() {
protected val scope: CoroutineScope = MainScope()

// TODO(b/174432505): Don't use the GLOBAL_INSTANCE, accept as a constructor param instead.
private val storageKeyManager = StorageKeyManager.GLOBAL_INSTANCE

// TODO: remove after G3 fixed
abstract val arcHost: ArcHost

Expand All @@ -32,7 +36,7 @@ abstract class ArcHostService : LifecycleService() {
open val arcHosts: List<ArcHost> by lazy { listOf(arcHost) }

val arcHostHelper: ArcHostHelper by lazy {
ArcHostHelper(this, *arcHosts.toTypedArray())
ArcHostHelper(this, storageKeyManager, *arcHosts.toTypedArray())
}

override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
Expand Down
6 changes: 5 additions & 1 deletion javatests/arcs/android/common/resurrection/DbHelperTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import android.content.ComponentName
import android.os.PersistableBundle
import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
import arcs.core.storage.StorageKeyManager
import arcs.core.storage.keys.RamDiskStorageKey
import com.google.common.truth.Truth.assertThat
import org.junit.After
Expand Down Expand Up @@ -57,7 +58,10 @@ class DbHelperTest {

@Before
fun setUp() {
dbHelper = DbHelper(ApplicationProvider.getApplicationContext())
dbHelper = DbHelper(
ApplicationProvider.getApplicationContext(),
StorageKeyManager.GLOBAL_INSTANCE
)
}

@After
Expand Down
5 changes: 2 additions & 3 deletions javatests/arcs/android/host/ArcHostHelperTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ import arcs.core.host.ArcStateChangeCallback
import arcs.core.host.ArcStateChangeRegistration
import arcs.core.host.ParticleIdentifier
import arcs.core.storage.StorageKey
import arcs.core.storage.StorageKeyManager
import arcs.core.storage.keys.VolatileStorageKey
import arcs.core.storage.testutil.DummyStorageKey
import arcs.core.storage.testutil.DummyStorageKeyManager
import arcs.core.util.guardedBy
import arcs.sdk.android.labs.host.ArcHostHelper
import arcs.sdk.android.labs.host.ResurrectableHost
Expand Down Expand Up @@ -170,8 +170,7 @@ class ArcHostHelperTest {
context = InstrumentationRegistry.getInstrumentation().targetContext
service = Robolectric.setupService(TestAndroidArcHostService::class.java)
arcHost = TestArcHost(context)
helper = ArcHostHelper(service, arcHost)
StorageKeyManager.GLOBAL_INSTANCE.addParser(DummyStorageKey)
helper = ArcHostHelper(service, DummyStorageKeyManager(), arcHost)
}

@Test
Expand Down
6 changes: 5 additions & 1 deletion javatests/arcs/android/host/TestExternalArcHostService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import arcs.core.host.ParticleRegistration
import arcs.core.host.SchedulerProvider
import arcs.core.host.SimpleSchedulerProvider
import arcs.core.host.TestingHost
import arcs.core.storage.StorageKeyManager
import arcs.sdk.android.labs.host.ArcHostHelper
import arcs.sdk.android.labs.host.ResurrectableHost
import arcs.sdk.android.storage.AndroidStorageServiceEndpointManager
Expand All @@ -30,8 +31,11 @@ abstract class TestExternalArcHostService : Service() {

val schedulerProvider = SimpleSchedulerProvider(Dispatchers.Default)

// TODO(b/174432505): Don't use the GLOBAL_INSTANCE, use a test-specific instance.
private val storageKeyManager = StorageKeyManager.GLOBAL_INSTANCE

private val arcHostHelper: ArcHostHelper by lazy {
ArcHostHelper(this, arcHost)
ArcHostHelper(this, storageKeyManager, arcHost)
}

override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
import arcs.android.common.map
import arcs.android.common.transaction
import arcs.core.storage.testutil.DummyStorageKeyManager
import com.google.common.truth.Truth.assertThat
import org.junit.Test
import org.junit.runner.RunWith
Expand All @@ -44,6 +45,7 @@ class DatabaseDowngradeTest {

val databaseImpl = DatabaseImpl(
ApplicationProvider.getApplicationContext(),
DummyStorageKeyManager(),
"arcs",
true
)
Expand Down
8 changes: 7 additions & 1 deletion javatests/arcs/android/storage/database/DatabaseImplTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import arcs.core.storage.database.DatabaseClient
import arcs.core.storage.database.DatabaseData
import arcs.core.storage.database.ReferenceWithVersion
import arcs.core.storage.testutil.DummyStorageKey
import arcs.core.storage.testutil.DummyStorageKeyManager
import arcs.core.testutil.assertSuspendingThrows
import arcs.core.util.ArcsDuration
import arcs.core.util.ArcsInstant
Expand Down Expand Up @@ -64,7 +65,11 @@ class DatabaseImplTest {

@Before
fun setUp() {
database = DatabaseImpl(ApplicationProvider.getApplicationContext(), "test.sqlite3")
database = DatabaseImpl(
ApplicationProvider.getApplicationContext(),
DummyStorageKeyManager(),
"test.sqlite3"
)
db = database.writableDatabase
StorageKeyManager.GLOBAL_INSTANCE.addParser(DummyStorageKey)
}
Expand Down Expand Up @@ -3784,6 +3789,7 @@ class DatabaseImplTest {
// Makes sure in memory database can also return valid size.
val inMemoryDatabase = DatabaseImpl(
ApplicationProvider.getApplicationContext(),
DummyStorageKeyManager(),
"test.sqlite3",
persistent = false
)
Expand Down