Skip to content

Commit b46f85f

Browse files
committed
Make robot discovery tolerate scope destruction
Robot lookup can overlap scope cleanup after navigation and fail while reading services or children. Skip those failures when the scope has been destroyed, and discard destroyed candidates before checking for ambiguity. Preserve ancestor robot lookup and propagate failures from live scopes, robot getters, and robot factories unchanged. Keep the fix in `:robot:public` without changing `:scope` or its synchronization. Lookup does not extend a scope's lifetime.
1 parent 235101e commit b46f85f

3 files changed

Lines changed: 236 additions & 17 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
### Fixed
1616

17+
- Skip scopes destroyed during robot lookup.
1718
- Allow the default Metro compiler integration to use `@ContributesBinding` on `Scoped` classes with an explicit qualified set binding.
1819

1920
### Security

‎robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt‎

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,8 @@ public interface Robot {
8181
*
8282
* [rootScope] refers to the application scope. The scope and all of its children are searched for
8383
* the contributed robot. Usually, the parameter doesn't need to be changed and the default value
84-
* can be used.
84+
* can be used. Scopes destroyed during discovery are skipped. Lookup does not keep scopes alive
85+
* while the robot runs.
8586
*/
8687
public inline fun <reified T : Robot> robot(
8788
rootScope: Scope = software.ralf.app.platform.robot.internal.rootScope,
@@ -103,39 +104,59 @@ public inline fun <reified T : Robot> robot(
103104

104105
@PublishedApi
105106
internal fun Scope.findRobotFactory(robotType: KClass<*>): (() -> Robot)? {
106-
var match: (() -> Robot)? = null
107-
var matchingScope: Scope? = null
107+
val matches = mutableMapOf<Scope, () -> Robot>()
108108

109109
fun Scope.findRobot(inheritedRobotTypes: Set<KClass<*>>) {
110-
if (isDestroyed()) return
110+
if (isDestroyed()) {
111+
return
112+
}
111113

112-
val robots = allRobots
113-
if (robotType !in inheritedRobotTypes) {
114-
robots[robotType]?.let { robotFactory ->
115-
check(match == null) {
116-
"Found Robot of type $robotType in multiple scopes: " +
117-
"${matchingScope?.name} and $name."
118-
}
119-
match = robotFactory
120-
matchingScope = this
114+
val component: RobotComponent?
115+
val graph: RobotGraph?
116+
val childScopes: Set<Scope>
117+
try {
118+
component = kotlinInjectComponentOrNull()
119+
graph = metroDependencyGraphOrNull()
120+
childScopes = children()
121+
} catch (exception: IllegalStateException) {
122+
// Destruction can finish after the initial check or between these reads.
123+
if (isDestroyed()) {
124+
return
121125
}
126+
throw exception
127+
}
128+
129+
val robots = robotFactories(component, graph)
130+
if (robotType !in inheritedRobotTypes) {
131+
robots[robotType]?.let { matches[this] = it }
122132
}
123133

124134
val visibleRobotTypes = inheritedRobotTypes + robots.keys
125-
children().forEach { childScope -> childScope.findRobot(visibleRobotTypes) }
135+
childScopes.forEach { childScope -> childScope.findRobot(visibleRobotTypes) }
126136
}
127137

128138
findRobot(emptySet())
129-
return match
139+
140+
val liveMatches = matches.filterKeys { !it.isDestroyed() }
141+
check(liveMatches.size <= 1) {
142+
"Found Robot of type $robotType in multiple scopes: " +
143+
"${liveMatches.keys.joinToString(" and ") { it.name }}."
144+
}
145+
return liveMatches.values.singleOrNull()
130146
}
131147

132148
@PublishedApi
133149
internal val Scope.allRobots: Map<KClass<*>, () -> Robot>
134150
get() {
135-
return kotlinInjectComponentOrNull<RobotComponent>()?.robots.orEmpty() +
136-
metroDependencyGraphOrNull<RobotGraph>()?.robots.orEmpty().mapValues { { it.value() } }
151+
return robotFactories(kotlinInjectComponentOrNull(), metroDependencyGraphOrNull())
137152
}
138153

154+
private fun robotFactories(
155+
component: RobotComponent?,
156+
graph: RobotGraph?,
157+
): Map<KClass<*>, () -> Robot> =
158+
component?.robots.orEmpty() + graph?.robots.orEmpty().mapValues { { it.value() } }
159+
139160
private inline fun <reified T : Any> Scope.metroDependencyGraphOrNull(): T? {
140161
return try {
141162
metroDependencyGraph<T>()

‎robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import assertk.assertions.contains
55
import assertk.assertions.isFalse
66
import assertk.assertions.isNotNull
77
import assertk.assertions.isNotSameInstanceAs
8+
import assertk.assertions.isSameInstanceAs
89
import assertk.assertions.isTrue
910
import kotlin.reflect.KClass
1011
import kotlin.test.Test
@@ -173,6 +174,202 @@ class RobotTest {
173174
assertFailsWith<IllegalStateException> { robot<ChildTestRobot>(rootScope) {} }
174175
}
175176

177+
@Test
178+
fun `scopes destroyed before their services are read are skipped`() {
179+
val expectedRobot = MetroTestRobot()
180+
val rootScope = rootScope(kiRobot = null, metroRobot = expectedRobot)
181+
val childScope = rootScope.buildChild("child")
182+
val destroyingChild =
183+
object : Scope by childScope {
184+
override fun isDestroyed(): Boolean {
185+
val destroyed = childScope.isDestroyed()
186+
childScope.destroy()
187+
return destroyed
188+
}
189+
}
190+
val scope =
191+
object : Scope by rootScope {
192+
override fun children(): Set<Scope> = setOf(destroyingChild)
193+
}
194+
195+
robot<MetroTestRobot>(scope) { assertThat(this).isSameInstanceAs(expectedRobot) }
196+
assertThat(childScope.isDestroyed()).isTrue()
197+
}
198+
199+
@Test
200+
fun `scopes destroyed before their children are read are skipped`() {
201+
val expectedRobot = MetroTestRobot()
202+
val rootScope = rootScope(kiRobot = null, metroRobot = expectedRobot)
203+
val childScope = rootScope.buildChild("child")
204+
val destroyingChild =
205+
object : Scope by childScope {
206+
override fun children(): Set<Scope> {
207+
childScope.destroy()
208+
return childScope.children()
209+
}
210+
}
211+
val scope =
212+
object : Scope by rootScope {
213+
override fun children(): Set<Scope> = setOf(destroyingChild)
214+
}
215+
216+
robot<MetroTestRobot>(scope) { assertThat(this).isSameInstanceAs(expectedRobot) }
217+
assertThat(childScope.isDestroyed()).isTrue()
218+
}
219+
220+
@Test
221+
fun `service failures from a live scope propagate unchanged`() {
222+
val failure = IllegalStateException("Service lookup failed")
223+
val scope =
224+
object : Scope by rootScope() {
225+
override fun <T : Any> getService(key: String): T? {
226+
throw failure
227+
}
228+
}
229+
230+
val actual = assertFailsWith<IllegalStateException> { robot<MetroTestRobot>(scope) {} }
231+
232+
assertThat(actual).isSameInstanceAs(failure)
233+
assertThat(scope.isDestroyed()).isFalse()
234+
}
235+
236+
@Test
237+
fun `children failures from a live scope propagate unchanged`() {
238+
val failure = IllegalStateException("Children lookup failed")
239+
val scope =
240+
object : Scope by rootScope() {
241+
override fun children(): Set<Scope> {
242+
throw failure
243+
}
244+
}
245+
246+
val actual = assertFailsWith<IllegalStateException> { robot<MetroTestRobot>(scope) {} }
247+
248+
assertThat(actual).isSameInstanceAs(failure)
249+
assertThat(scope.isDestroyed()).isFalse()
250+
}
251+
252+
@Test
253+
fun `scopes destroyed while reading robots do not hide a sibling robot`() {
254+
val rootScope = rootScope(kiRobot = null, metroRobot = null)
255+
lateinit var destroyingChild: Scope
256+
destroyingChild =
257+
rootScope.buildChild("destroying-child") {
258+
addMetroDependencyGraph(
259+
object : RobotGraph {
260+
override val robots: Map<KClass<*>, () -> Robot>
261+
get() {
262+
destroyingChild.destroy()
263+
return mapOf(ChildTestRobot::class to { ChildTestRobot() })
264+
}
265+
},
266+
)
267+
}
268+
val expectedRobot = ChildTestRobot()
269+
rootScope.buildChild("live-child") { addMetroDependencyGraph(Graph(expectedRobot)) }
270+
271+
robot<ChildTestRobot>(rootScope) { assertThat(this).isSameInstanceAs(expectedRobot) }
272+
assertThat(destroyingChild.isDestroyed()).isTrue()
273+
}
274+
275+
@Test
276+
fun `a matching scope destroyed later during discovery is ignored`() {
277+
val rootScope = rootScope(kiRobot = null, metroRobot = null)
278+
val firstChild =
279+
rootScope.buildChild("first-child") { addMetroDependencyGraph(Graph(ChildTestRobot())) }
280+
val expectedRobot = ChildTestRobot()
281+
rootScope.buildChild("second-child") {
282+
addMetroDependencyGraph(
283+
object : RobotGraph {
284+
override val robots: Map<KClass<*>, () -> Robot>
285+
get() {
286+
firstChild.destroy()
287+
return mapOf(ChildTestRobot::class to { expectedRobot })
288+
}
289+
},
290+
)
291+
}
292+
293+
robot<ChildTestRobot>(rootScope) { assertThat(this).isSameInstanceAs(expectedRobot) }
294+
assertThat(firstChild.isDestroyed()).isTrue()
295+
}
296+
297+
@Test
298+
fun `robot getter failures propagate even if the scope was destroyed`() {
299+
val rootScope = rootScope(kiRobot = null, metroRobot = null)
300+
val failure = IllegalStateException("Robot getter failed")
301+
lateinit var childScope: Scope
302+
childScope =
303+
rootScope.buildChild("child") {
304+
addMetroDependencyGraph(
305+
object : RobotGraph {
306+
override val robots: Map<KClass<*>, () -> Robot>
307+
get() {
308+
childScope.destroy()
309+
throw failure
310+
}
311+
},
312+
)
313+
}
314+
315+
val actual = assertFailsWith<IllegalStateException> { robot<ChildTestRobot>(rootScope) {} }
316+
317+
assertThat(actual).isSameInstanceAs(failure)
318+
}
319+
320+
@Test
321+
fun `robot getters accessing another destroyed scope propagate the failure`() {
322+
val destroyedScope = Scope.buildRootScope("destroyed")
323+
destroyedScope.destroy()
324+
var failure: IllegalStateException? = null
325+
val rootScope = Scope.buildRootScope {
326+
addMetroDependencyGraph(
327+
object : RobotGraph {
328+
override val robots: Map<KClass<*>, () -> Robot>
329+
get() =
330+
try {
331+
destroyedScope.getService<Map<KClass<*>, () -> Robot>>("robots").orEmpty()
332+
} catch (exception: IllegalStateException) {
333+
failure = exception
334+
throw exception
335+
}
336+
},
337+
)
338+
}
339+
340+
val actual = assertFailsWith<IllegalStateException> { robot<ChildTestRobot>(rootScope) {} }
341+
342+
assertThat(actual).isSameInstanceAs(failure)
343+
assertThat(rootScope.isDestroyed()).isFalse()
344+
}
345+
346+
@Test
347+
fun `robot factory failures propagate unchanged`() {
348+
val failure = IllegalStateException("Robot factory failed")
349+
val rootScope = Scope.buildRootScope {
350+
addKotlinInjectComponent(
351+
object : RobotComponent {
352+
override val robots: Map<KClass<out Robot>, () -> Robot> =
353+
mapOf(KiTestRobot::class to { throw failure })
354+
},
355+
)
356+
}
357+
358+
val actual = assertFailsWith<IllegalStateException> { robot<KiTestRobot>(rootScope) {} }
359+
360+
assertThat(actual).isSameInstanceAs(failure)
361+
}
362+
363+
@Test
364+
fun `discovery starting at a child finds robots from its parent`() {
365+
val kiRobot = KiTestRobot()
366+
val metroRobot = MetroTestRobot()
367+
val childScope = rootScope(kiRobot, metroRobot).buildChild("child")
368+
369+
robot<KiTestRobot>(childScope) { assertThat(this).isSameInstanceAs(kiRobot) }
370+
robot<MetroTestRobot>(childScope) { assertThat(this).isSameInstanceAs(metroRobot) }
371+
}
372+
176373
private fun rootScope(
177374
kiRobot: Robot? = KiTestRobot(),
178375
metroRobot: Robot? = MetroTestRobot(),

0 commit comments

Comments
 (0)