Out of scope Controllers / Views don't get cleaned up after they become unreachable when using Workspaces

[adam-arold]

I'm using a Workspace and dockInNewScope to navigate between parent and child views. My problem is that if I subscribe to an event in a child view / controller it will get fired even if I navigate away from that view and It becomes effectively unreachable (meaning that it is not reachable from code).

Here is the code to reproduce the problem:

private val logger = LoggerFactory.getLogger("main")

fun main(args: Array<String>) {
    Application.launch(TestApp::class.java)
}

object TestEvent : FXEvent()

class ParentModel : ViewModel() {
    val id = UUID.randomUUID().toString().substring(0, 4)
}

class ChildModel : ViewModel() {
    val id = UUID.randomUUID().toString().substring(0, 4)
}

class TestApp : App(Workspace::class) {

    override fun onBeforeShow(view: UIComponent) {
        workspace.dockInNewScope<ParentView>(ParentModel())
    }
}

class ParentView : View("parent") {

    val id = UUID.randomUUID().toString().substring(0, 4)

    private val model: ParentModel by inject()
    private val controller: ParentController by inject()

    override val root = hbox {
        button("new child") {
            action {
                workspace.dockInNewScope<ChildView>(ChildModel())
            }
        }
    }

    override fun onDock() {
        logger.info("Docking. M: ${model.id}, V: $id, C: ${controller.id}")
        controller.onDock()
        logger.info("")
    }

    override fun onUndock() {
        logger.info("Undocking. M: ${model.id}, V: $id, C: ${controller.id}")
        controller.onUndock()
        logger.info("")
    }

}

class ParentController : Controller() {

    val id = UUID.randomUUID().toString().substring(0, 4)

    private val model: ParentModel by inject()
    private val view: ParentView by inject()

    fun onDock() {
        logger.info("Docking. M: ${model.id}, V: ${view.id}, C: $id")
    }

    fun onUndock() {
        logger.info("Undocking. M: ${model.id}, V: ${view.id}, C: $id")
    }

}

class ChildView : View("child") {

    val id = UUID.randomUUID().toString().substring(0, 4)

    private val model: ChildModel by inject()
    private val controller: ChildController by inject()

    override val root = hbox {
        text("child")
        button("select local file") {
            logger.info("Firing test event.")
            action {
                fire(TestEvent)
            }
        }
    }

    override fun onDock() {
        logger.info("Docking. M: ${model.id}, V: $id, C: ${controller.id}")
        controller.onDock()
        logger.info("")
    }

    override fun onUndock() {
        logger.info("Undocking. M: ${model.id}, V: $id, C: ${controller.id}")
        workspace.viewStack.remove(this)
        controller.onUndock()
        logger.info("")
    }

}

class ChildController : Controller() {

    val id = UUID.randomUUID().toString().substring(0, 4)

    private val model: ChildModel by inject()
    private val view: ChildView by inject()

    init {
        logger.info("Subscribing to test event.")
        subscribe<TestEvent> {
            logger.info("======= TEST EVENT")
        }
    }

    fun onDock() {
        logger.info("Docking. M: ${model.id}, V: ${view.id}, C: $id")
    }

    fun onUndock() {
        logger.info("Undocking. M: ${model.id}, V: ${view.id}, C: $id")
    }

}

Now if I follow this sequence:

[Click "new child"] -> [Click "select local file"] -> [Click "back"] -> [Click "new child"] -> [Click "select local file"]

this is what will get printed:

14:33:15.727 [JavaFX Application Thread] INFO main - Docking. M: f38b, V: 9f08, C: fd47 14:33:15.730 [JavaFX Application Thread] INFO main - Docking. M: f38b, V: 9f08, C: fd47 14:33:15.730 [JavaFX Application Thread] INFO main - 14:33:17.854 [JavaFX Application Thread] INFO main - Firing test event. 14:33:17.857 [JavaFX Application Thread] INFO main - Undocking. M: f38b, V: 9f08, C: fd47 14:33:17.857 [JavaFX Application Thread] INFO main - Undocking. M: f38b, V: 9f08, C: fd47 14:33:17.857 [JavaFX Application Thread] INFO main - 14:33:17.859 [JavaFX Application Thread] INFO main - Subscribing to test event. 14:33:17.860 [JavaFX Application Thread] INFO main - Docking. M: 250d, V: dc5a, C: bef2 14:33:17.860 [JavaFX Application Thread] INFO main - Docking. M: 250d, V: dc5a, C: bef2 14:33:17.860 [JavaFX Application Thread] INFO main - 14:33:19.019 [JavaFX Application Thread] INFO main - ======= TEST EVENT 14:33:21.620 [JavaFX Application Thread] INFO main - Undocking. M: 250d, V: dc5a, C: bef2 14:33:21.620 [JavaFX Application Thread] INFO main - Undocking. M: 250d, V: dc5a, C: bef2 14:33:21.620 [JavaFX Application Thread] INFO main - 14:33:21.620 [JavaFX Application Thread] INFO main - Docking. M: f38b, V: 9f08, C: fd47 14:33:21.620 [JavaFX Application Thread] INFO main - Docking. M: f38b, V: 9f08, C: fd47 14:33:21.620 [JavaFX Application Thread] INFO main - 14:33:23.843 [JavaFX Application Thread] INFO main - Firing test event. 14:33:23.844 [JavaFX Application Thread] INFO main - Undocking. M: f38b, V: 9f08, C: fd47 14:33:23.844 [JavaFX Application Thread] INFO main - Undocking. M: f38b, V: 9f08, C: fd47 14:33:23.844 [JavaFX Application Thread] INFO main - 14:33:23.845 [JavaFX Application Thread] INFO main - Subscribing to test event. 14:33:23.845 [JavaFX Application Thread] INFO main - Docking. M: 7201, V: 0405, C: 3770 14:33:23.845 [JavaFX Application Thread] INFO main - Docking. M: 7201, V: 0405, C: 3770 14:33:23.845 [JavaFX Application Thread] INFO main - 14:33:25.226 [JavaFX Application Thread] INFO main - ======= TEST EVENT 14:33:25.226 [JavaFX Application Thread] INFO main - ======= TEST EVENT

So this means that only one ChildView gets docked when I go back but the subscription of the old View which is unreachable is still active. There is a workaround for this if I do this in the ChildController:

class ChildController : Controller() {

    val id = UUID.randomUUID().toString().substring(0, 4)

    private val model: ChildModel by inject()
    private val view: ChildView by inject()
    private val subscriptions = mutableListOf<FXEventRegistration>()

    init {
        logger.info("Subscribing to test event.")
        subscriptions.add(subscribe<TestEvent> {
            logger.info("======= TEST EVENT")
        })
    }

    fun onDock() {
        logger.info("Docking. M: ${model.id}, V: ${view.id}, C: $id")
    }

    fun onUndock() {
        logger.info("Undocking. M: ${model.id}, V: ${view.id}, C: $id")
        subscriptions.unsubscribeAll()
    }

}

fun List<FXEventRegistration>.unsubscribeAll() {
    forEach { it.unsubscribe() }
}

but this still seems like a memory leak to me. What am I doing wrong?

[edvin]

The issue here is that only UIComponents are undocked, and hence only subscriptions made in UIComponents will be unregistered. If you move your subscription from the Controller to the View you should observe that the registration is unregistered when the view is undocked.

What we need seems to be some way of saying that a controller injected by a view should be unregistered when the view is undocked. I think this could be done automatically (be the default) for every UIComponent not in the default scope. These are just my preliminary thoughts.

[adam-arold]

My thinking here is that it should be a Controller's responsibility to add those listeners. The View should be just a dumb list of components which delegates to the Controller. This is how MVC is supposed to work. Am I doing it wrong? I don't want to mix View code with behavior, that's why I have it in my Controller. This is the reason why I have onDock methods in my Controllers as well.

[edvin]

I absolutely agree with you :)

[adam-arold]

There is this thing which puzzles me: it would make sense to have onDock in the Controller instead of the View because the Controller is supposed to tamper with the View and not the other way around, but in that case the right name should not be onDock, because a Controller is an abstract thing, it is not docked. What do you think? How can I have this inversion of control with TornadoFX?

[edvin]

The name Controller is a bit misleading in TornadoFX, because it implies that it's the view controller, but it's not. A controller is more like a service. The View is infact the ViewController in TornadoFX. In any case, we need to have a mechanism to be able to unsubscribe automatically from controllers, so I'll look into this ASAP and get this fixed :)

[adam-arold]

Do you have a best practices document somewhere? What do you mean by ViewController? Having both the View and the Controller code in one place would quickly lead to convoluted code I think.

[edvin]

The View class should contain the code that builds your UI as well as the code that reacts to events from the UI. Business logic should be placed in the controller. If you use ViewModels you can opt to put view related logic there as well.