Repository: cashapp/redwood (kotlin)

Issue:
how to handle race where an incoming event targets this removed node?
https://github.com/cashapp/redwood/blob/aec88df2189e46fbcaff9192d76a50607f0fd1a7/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolState.kt#L93

```text

/*
 * Copyright (C) 2022 Square, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package app.cash.redwood.protocol.compose

import app.cash.redwood.protocol.ChildrenDiff
import app.cash.redwood.protocol.ChildrenTag
import app.cash.redwood.protocol.Diff
import app.cash.redwood.protocol.Event
import app.cash.redwood.protocol.Id
import app.cash.redwood.protocol.LayoutModifiers
import app.cash.redwood.protocol.PropertyDiff
import app.cash.redwood.widget.Widget

/** @suppress For generated code use only. */
public class ProtocolState {
  private var nextValue = Id.Root.value + 1L
  private val nodes = mutableMapOf<Id, DiffProducingWidget>()

  private var childrenDiffs = mutableListOf<ChildrenDiff>()
  private var layoutModifiers = mutableListOf<LayoutModifiers>()
  private var propertyDiffs = mutableListOf<PropertyDiff>()

  public fun nextId(): Id {
    val value = nextValue
    nextValue = value + 1L
    return Id(value)
  }

  public fun append(childrenDiff: ChildrenDiff) {
    childrenDiffs += childrenDiff
  }

  public fun append(layoutModifiers: LayoutModifiers) {
    this.layoutModifiers += layoutModifiers
  }

  public fun append(propertyDiff: PropertyDiff) {
    propertyDiffs += propertyDiff
  }

  /**
   * If there were any calls to [append] since the last call to this function return them as a
   * [Diff] and reset the internal lists to be empty. This function returns null if there were
   * no calls to [append] since the last invocation.
   */
  public fun createDiffOrNull(): Diff? {
    val existingChildrenDiffs = childrenDiffs
    val existingLayoutModifierDiffs = layoutModifiers
    val existingPropertyDiffs = propertyDiffs
    if (existingPropertyDiffs.isNotEmpty() || existingLayoutModifierDiffs.isNotEmpty() || existingChildrenDiffs.isNotEmpty()) {
      childrenDiffs = mutableListOf()
      layoutModifiers = mutableListOf()
      propertyDiffs = mutableListOf()

      return Diff(
        childrenDiffs = existingChildrenDiffs,
        layoutModifiers = existingLayoutModifierDiffs,
        propertyDiffs = existingPropertyDiffs,
      )
    }
    return null
  }

  public fun addWidget(widget: DiffProducingWidget) {
    check(nodes.put(widget.id, widget) == null) {
      "Attempted to add widget with ID ${widget.id.value} but one already exists"
    }
  }

  public fun removeWidget(id: Id) {
    nodes.remove(id)
  }

  public fun widgetChildren(id: Id, tag: ChildrenTag): Widget.Children<Nothing> {
    return DiffProducingWidgetChildren(id, tag, this)
  }

  public fun sendEvent(event: Event) {
    val node = checkNotNull(nodes[event.id]) {
      // TODO how to handle race where an incoming event targets this removed node?
      "Unknown node ${event.id} for event with tag ${event.tag}"
    }
    node.sendEvent(event)
  }
}

```

Pull request:
Closes #646

Patch:
diff --git a/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandler.kt b/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandler.kt
index 2869cf0a46..381c490b7c 100644
--- a/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandler.kt
+++ b/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandler.kt
@@ -16,6 +16,7 @@
 package app.cash.redwood.protocol.compose
 
 import app.cash.redwood.protocol.EventTag
+import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.WidgetTag
 import kotlin.jvm.JvmField
 
@@ -28,6 +29,9 @@ public interface ProtocolMismatchHandler {
   /** Handle a request to process an unknown event [tag] for the specified widget [widgetTag]. */
   public fun onUnknownEvent(widgetTag: WidgetTag, tag: EventTag)
 
+  /** Handle an event whose node [id] is unknown. */
+  public fun onUnknownEventNode(id: Id, tag: EventTag)
+
   public companion object {
     /** A [ProtocolMismatchHandler] which throws [IllegalArgumentException] for all callbacks. */
     @JvmField
@@ -37,6 +41,12 @@ public interface ProtocolMismatchHandler {
           "Unknown event tag ${tag.value} for widget tag ${widgetTag.value}",
         )
       }
+
+      override fun onUnknownEventNode(id: Id, tag: EventTag) {
+        throw IllegalArgumentException(
+          "Unknown node ID ${id.value} for event with tag ${tag.value}",
+        )
+      }
     }
   }
 }
diff --git a/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolState.kt b/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolState.kt
index 88d0316d71..7cfcb158d8 100644
--- a/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolState.kt
+++ b/redwood-protocol-compose/src/commonMain/kotlin/app/cash/redwood/protocol/compose/ProtocolState.kt
@@ -18,7 +18,6 @@ package app.cash.redwood.protocol.compose
 import app.cash.redwood.protocol.ChildrenDiff
 import app.cash.redwood.protocol.ChildrenTag
 import app.cash.redwood.protocol.Diff
-import app.cash.redwood.protocol.Event
 import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.LayoutModifiers
 import app.cash.redwood.protocol.PropertyDiff
@@ -84,15 +83,9 @@ public class ProtocolState {
     nodes.remove(id)
   }
 
+  public fun getWidget(id: Id): DiffProducingWidget? = nodes[id]
+
   public fun widgetChildren(id: Id, tag: ChildrenTag): Widget.Children<Nothing> {
     return DiffProducingWidgetChildren(id, tag, this)
   }
-
-  public fun sendEvent(event: Event) {
-    val node = checkNotNull(nodes[event.id]) {
-      // TODO how to handle race where an incoming event targets this removed node?
-      "Unknown node ${event.id} for event with tag ${event.tag}"
-    }
-    node.sendEvent(event)
-  }
 }
diff --git a/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/diffProducingGeneration.kt b/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/diffProducingGeneration.kt
index 51b31f37ef..a7668df1e5 100644
--- a/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/diffProducingGeneration.kt
+++ b/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/diffProducingGeneration.kt
@@ -41,12 +41,20 @@ import com.squareup.kotlinpoet.joinToCode
 
 /*
 class SunspotProtocolBridge(
-  private val bridge: ProtocolBridge,
+  private val state: ProtocolState,
+  private val mismatchHandler: ProtocolMismatchHandler,
   override val root: Widget.Children<Nothing>,
   override val provider: SunspotWidgetFactoryProvider<Nothing>,
 ) : ProtocolBridge {
-  override fun createDiffOrNull(): Diff? = bridge.createDiffOrNull()
-  override fun sendEvent(event: Event) = bridge.sendEvent(event)
+  override fun createDiffOrNull(): Diff? = state.createDiffOrNull()
+  override fun sendEvent(event: Event) {
+    val node = state.getWidget(event.id)
+    if (node != null) {
+      node.sendEvent(event)
+    } else {
+      mismatchHandler.onUnknownEventNode(event.id, event.tag)
+    }
+  }
 
   companion object : ProtocolBridge.Factory {
     override fun create(
@@ -59,7 +67,7 @@ class SunspotProtocolBridge(
           Sunspot = DiffProducingSunspotWidgetFactory(bridge, json, mismatchHandler),
           RedwoodLayout = DiffProducingRedwoodLayoutWidgetFactory(bridge, json, mismatchHandler),
       )
-      return SunspotProtocolBridge(bridge, root, factories)
+      return SunspotProtocolBridge(bridge, mismatchHandler, root, factories)
     }
   }
 }
@@ -75,14 +83,20 @@ internal fun generateProtocolBridge(
         .addSuperinterface(ComposeProtocol.ProtocolBridge)
         .primaryConstructor(
           FunSpec.constructorBuilder()
-            .addParameter("bridge", ComposeProtocol.ProtocolState)
+            .addParameter("state", ComposeProtocol.ProtocolState)
+            .addParameter("mismatchHandler", ComposeProtocol.ProtocolMismatchHandler)
             .addParameter("root", RedwoodWidget.WidgetChildren.parameterizedBy(NOTHING))
             .addParameter("provider", providerType)
             .build(),
         )
         .addProperty(
-          PropertySpec.builder("bridge", ComposeProtocol.ProtocolState, PRIVATE)
-            .initializer("bridge")
+          PropertySpec.builder("state", ComposeProtocol.ProtocolState, PRIVATE)
+            .initializer("state")
+            .build(),
+        )
+        .addProperty(
+          PropertySpec.builder("mismatchHandler", ComposeProtocol.ProtocolMismatchHandler, PRIVATE)
+            .initializer("mismatchHandler")
             .build(),
         )
         .addProperty(
@@ -99,7 +113,7 @@ internal fun generateProtocolBridge(
           FunSpec.builder("createDiffOrNull")
             .addModifiers(OVERRIDE)
             .returns(Protocol.Diff.copy(nullable = true))
-            .addStatement("return bridge.createDiffOrNull()")
+            .addStatement("return state.createDiffOrNull()")
             .build(),
         )
         .addFunction(
@@ -107,7 +121,12 @@ internal fun generateProtocolBridge(
             .addModifiers(OVERRIDE)
             .returns(UNIT)
             .addParameter("event", Protocol.Event)
-            .addStatement("return bridge.sendEvent(event)")
+            .addStatement("val node = state.getWidget(event.id)")
+            .beginControlFlow("if (node != null)")
+            .addStatement("node.sendEvent(event)")
+            .nextControlFlow("else")
+            .addStatement("mismatchHandler.onUnknownEventNode(event.id, event.tag)")
+            .endControlFlow()
             .build(),
         )
         .addType(
@@ -129,7 +148,7 @@ internal fun generateProtocolBridge(
                   }
                   addStatement("val factories = %T(\n%L)", schema.getWidgetFactoriesType(), arguments.joinToCode(separator = ",\n"))
                 }
-                .addStatement("return %T(bridge, root, factories)", type)
+                .addStatement("return %T(bridge, mismatchHandler, root, factories)", type)
                 .build(),
             )
             .build(),


Test patch:
diff --git a/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/DiffProducingWidgetFactoryTest.kt b/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/GeneratedProtocolBridgeTest.kt
similarity index 87%
rename from redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/DiffProducingWidgetFactoryTest.kt
rename to redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/GeneratedProtocolBridgeTest.kt
index 148e2eb8a8..4f186dafa5 100644
--- a/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/DiffProducingWidgetFactoryTest.kt
+++ b/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/GeneratedProtocolBridgeTest.kt
@@ -37,7 +37,7 @@ import kotlinx.serialization.json.JsonPrimitive
 import kotlinx.serialization.json.buildJsonObject
 import kotlinx.serialization.modules.SerializersModule
 
-class DiffProducingWidgetFactoryTest {
+class GeneratedProtocolBridgeTest {
   @Test fun propertyUsesSerializersModule() {
     val json = Json {
       serializersModule = SerializersModule {
@@ -144,9 +144,8 @@ class DiffProducingWidgetFactoryTest {
     val bridge = ExampleSchemaProtocolBridge.create()
     val button = bridge.provider.ExampleSchema.Button() as DiffProducingWidget
 
-    val event = Event(Id(1), EventTag(3456543))
     val t = assertFailsWith<IllegalArgumentException> {
-      button.sendEvent(event)
+      button.sendEvent(Event(Id(1), EventTag(3456543)))
     }
 
     assertEquals("Unknown event tag 3456543 for widget tag 4", t.message)
@@ -161,4 +160,21 @@ class DiffProducingWidgetFactoryTest {
 
     assertEquals("Unknown event 3456543 for 4", handler.events.single())
   }
+
+  @Test fun unknownEventNodeThrowsDefault() {
+    val bridge = ExampleSchemaProtocolBridge.create()
+    val t = assertFailsWith<IllegalArgumentException> {
+      bridge.sendEvent(Event(Id(3456543), EventTag(1)))
+    }
+    assertEquals("Unknown node ID 3456543 for event with tag 1", t.message)
+  }
+
+  @Test fun unknownEventNodeCallsHandler() {
+    val handler = RecordingProtocolMismatchHandler()
+    val bridge = ExampleSchemaProtocolBridge.create(mismatchHandler = handler)
+
+    bridge.sendEvent(Event(Id(3456543), EventTag(1)))
+
+    assertEquals("Unknown ID 3456543 for event tag 1", handler.events.single())
+  }
 }
diff --git a/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandlerTest.kt b/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandlerTest.kt
index 1516262450..2b3e1f9c51 100644
--- a/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandlerTest.kt
+++ b/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/ProtocolMismatchHandlerTest.kt
@@ -16,6 +16,7 @@
 package app.cash.redwood.protocol.compose
 
 import app.cash.redwood.protocol.EventTag
+import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.WidgetTag
 import kotlin.test.Test
 import kotlin.test.assertEquals
@@ -28,4 +29,11 @@ class ProtocolMismatchHandlerTest {
     }
     assertEquals("Unknown event tag 2 for widget tag 1", t.message)
   }
+
+  @Test fun throwingUnknownEventNode() {
+    val t = assertFailsWith<IllegalArgumentException> {
+      ProtocolMismatchHandler.Throwing.onUnknownEventNode(Id(1), EventTag(2))
+    }
+    assertEquals("Unknown node ID 1 for event with tag 2", t.message)
+  }
 }
diff --git a/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/RecordingProtocolMismatchHandler.kt b/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/RecordingProtocolMismatchHandler.kt
index 09b1383f0e..44d3dc8d23 100644
--- a/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/RecordingProtocolMismatchHandler.kt
+++ b/redwood-protocol-compose/src/commonTest/kotlin/app/cash/redwood/protocol/compose/RecordingProtocolMismatchHandler.kt
@@ -16,6 +16,7 @@
 package app.cash.redwood.protocol.compose
 
 import app.cash.redwood.protocol.EventTag
+import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.WidgetTag
 
 class RecordingProtocolMismatchHandler : ProtocolMismatchHandler {
@@ -24,4 +25,8 @@ class RecordingProtocolMismatchHandler : ProtocolMismatchHandler {
   override fun onUnknownEvent(widgetTag: WidgetTag, tag: EventTag) {
     events += "Unknown event ${tag.value} for ${widgetTag.value}"
   }
+
+  override fun onUnknownEventNode(id: Id, tag: EventTag) {
+    events += "Unknown ID ${id.value} for event tag ${tag.value}"
+  }
 }