From d61356ca9e65891b4165e8ddc818d9070bc27082 Mon Sep 17 00:00:00 2001
From: Mike Hearn <mike@r3cev.com>
Date: Thu, 4 Aug 2016 17:19:10 +0200
Subject: [PATCH] Don't let Kryo crawl into Rx subscriptions, and add a unit
 test to ensure this doesn't regress, as the rule is not intuitive.

---
 .../r3corda/core/utilities/ProgressTracker.kt | 13 ++++---
 .../core/utilities/ProgressTrackerTest.kt     | 34 +++++++++++++++++--
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/core/src/main/kotlin/com/r3corda/core/utilities/ProgressTracker.kt b/core/src/main/kotlin/com/r3corda/core/utilities/ProgressTracker.kt
index 7b4d2a1cb5..7305260f2a 100644
--- a/core/src/main/kotlin/com/r3corda/core/utilities/ProgressTracker.kt
+++ b/core/src/main/kotlin/com/r3corda/core/utilities/ProgressTracker.kt
@@ -80,8 +80,8 @@ class ProgressTracker(vararg steps: Step) {
 
     // This field won't be serialized.
     private val _changes by TransientProperty { PublishSubject.create<Change>() }
-
-    private val childProgressTrackers = HashMap<Step, Pair<ProgressTracker, Subscription>>()
+    private data class Child(val tracker: ProgressTracker, @Transient val subscription: Subscription?)
+    private val childProgressTrackers = HashMap<Step, Child>()
 
     init {
         steps.forEach {
@@ -130,19 +130,19 @@ class ProgressTracker(vararg steps: Step) {
     val currentStepRecursive: Step
         get() = getChildProgressTracker(currentStep)?.currentStepRecursive ?: currentStep
 
-    fun getChildProgressTracker(step: Step): ProgressTracker? = childProgressTrackers[step]?.first
+    fun getChildProgressTracker(step: Step): ProgressTracker? = childProgressTrackers[step]?.tracker
 
     fun setChildProgressTracker(step: ProgressTracker.Step, childProgressTracker: ProgressTracker) {
         val subscription = childProgressTracker.changes.subscribe({ _changes.onNext(it) }, { _changes.onError(it) })
-        childProgressTrackers[step] = Pair(childProgressTracker, subscription)
+        childProgressTrackers[step] = Child(childProgressTracker, subscription)
         childProgressTracker.parent = this
         _changes.onNext(Change.Structural(this, step))
     }
 
     private fun removeChildProgressTracker(step: ProgressTracker.Step) {
         childProgressTrackers.remove(step)?.let {
-            it.first.parent = null
-            it.second.unsubscribe()
+            it.tracker.parent = null
+            it.subscription?.unsubscribe()
         }
         _changes.onNext(Change.Structural(this, step))
     }
@@ -192,7 +192,6 @@ class ProgressTracker(vararg steps: Step) {
      * if a step changed its label or rendering).
      */
     val changes: Observable<Change> get() = _changes
-
 }
 
 
diff --git a/core/src/test/kotlin/com/r3corda/core/utilities/ProgressTrackerTest.kt b/core/src/test/kotlin/com/r3corda/core/utilities/ProgressTrackerTest.kt
index c6e736706e..8b1c99f2f3 100644
--- a/core/src/test/kotlin/com/r3corda/core/utilities/ProgressTrackerTest.kt
+++ b/core/src/test/kotlin/com/r3corda/core/utilities/ProgressTrackerTest.kt
@@ -1,5 +1,11 @@
 package com.r3corda.core.utilities
 
+import com.esotericsoftware.kryo.Kryo
+import com.esotericsoftware.kryo.KryoSerializable
+import com.esotericsoftware.kryo.io.Input
+import com.esotericsoftware.kryo.io.Output
+import com.r3corda.core.serialization.createKryo
+import com.r3corda.core.serialization.serialize
 import org.junit.Before
 import org.junit.Test
 import java.util.*
@@ -24,10 +30,12 @@ class ProgressTrackerTest {
     }
 
     lateinit var pt: ProgressTracker
+    lateinit var pt2: ProgressTracker
 
     @Before
     fun before() {
         pt = SimpleSteps.tracker()
+        pt2 = ChildSteps.tracker()
     }
 
     @Test
@@ -56,8 +64,6 @@ class ProgressTrackerTest {
 
     @Test
     fun `nested children are stepped correctly`() {
-        val pt2 = ChildSteps.tracker()
-
         val stepNotification = LinkedList<ProgressTracker.Change>()
         pt.changes.subscribe {
             stepNotification += it
@@ -82,10 +88,32 @@ class ProgressTrackerTest {
 
     @Test
     fun `can be rewound`() {
-        val pt2 = ChildSteps.tracker()
         pt.setChildProgressTracker(SimpleSteps.TWO, pt2)
         repeat(4) { pt.nextStep() }
         pt.currentStep = SimpleSteps.ONE
         assertEquals(SimpleSteps.TWO, pt.nextStep())
     }
+
+    @Test
+    fun rxSubscriptionsAreNotSerialized() {
+        class Unserializable : KryoSerializable {
+            override fun write(kryo: Kryo?, output: Output?) = throw AssertionError("not called")
+            override fun read(kryo: Kryo?, input: Input?) = throw AssertionError("not called")
+
+            fun foo() {
+                println("bar")
+            }
+        }
+        val kryo = createKryo().apply {
+            // This is required to make sure Kryo walks through the auto-generated members for the lambda below.
+            fieldSerializerConfig.isIgnoreSyntheticFields = false
+        }
+        pt.setChildProgressTracker(SimpleSteps.TWO, pt2)
+        class Tmp {
+            val unserializable = Unserializable()
+            val subscription = pt2.changes.subscribe { unserializable.foo() }
+        }
+        Tmp()
+        pt.serialize(kryo)
+    }
 }
\ No newline at end of file