<@UD17D4K8W> After the upgrade to 4.0.0, things se...

# ballast

[rocketraman]

@Casey Brooks After the upgrade to 4.0.0, things seem to be mostly working, but I am able to quite easily reproduce a ClosedSendChannelException in the browser, after which my VMs stop working. Its not clear from the console log where the exception is coming from. Is it perhaps an incompatibility with Kotlin 1.9.20 -- 4.0.0 appears to be compiled against 1.9.10?

[Casey Brooks]

Can you please provide a sample repo so I can take a look?

[rocketraman]

The exception appears to happen during a navigation, so I'm going to try to migrate my routing to ballast-navigation and see if the issue still happens. If it still does, I'll try to create a reproducer.

[rocketraman]

Yep, still happening with ballast-navigation.

[rocketraman]

Ok, the problem appears to be when doing this in a Composable:

LaunchedEffect(vm /* Or Unit */) {
    vm.send(Inputs.Initialize)
  }

Any thoughts on why this would be a problem @Casey Brooks? It worked fine with ballast 2.x.

[rocketraman]

Switching to sending the Inputs.Initialize via the BootstrapInterceptor results in the same error, though with better logging as the error is caught by Ballast:

[SignIn] Uncaught error (Channel was closed)
[SignIn] Ballast error capture, input sequence types are [] BallastLoggingException: ClosedSendChannelException: Channel was closed
    at protoOf.doResume_5yljmg_k$ (webpack-internal:///./kotlin/ballast-ballast-logging.js:336:31)
    at protoOf.invoke_2dr2y6_k$ (webpack-internal:///./kotlin/ballast-ballast-logging.js:302:16)
    at onEach$o$collect$slambda.l [as $action_1] (webpack-internal:///./kotlin/ballast-ballast-logging.js:631:16)
    at protoOf.doResume_5yljmg_k$ (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:16461:34)
    at protoOf.invoke_oz8tte_k$ (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:16443:16)
    at sam$kotlinx_coroutines_flow_FlowCollector$0_10.l [as function_1] (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:16502:16)
    at protoOf.emit_t92u1f_k$ (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:16432:17)
    at protoOf.emit_t92u1f_k$ (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:21105:29)
    at protoOf.doResume_5yljmg_k$ (webpack-internal:///./kotlin/ballast-ballast-api.js:4040:57)
    at protoOf.invoke_m1n198_k$ (webpack-internal:///./kotlin/ballast-ballast-api.js:4025:16)
Caused by: ClosedSendChannelException: Channel was closed
    at protoOf.get_sendException_qpq1ry_k$ (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:8743:37)
    at onClosedSend (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:5532:31)
    at protoOf.doResume_5yljmg_k$ (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:7733:37)
    at protoOf.send_44jogj_k$ (webpack-internal:///./kotlin/kotlinx.coroutines-kotlinx-coroutines-core-js-ir.js:8168:16)
    at protoOf.enqueue_9t430b_k$ (webpack-internal:///./kotlin/ballast-ballast-api.js:1950:30)
    at protoOf.doResume_5yljmg_k$ (webpack-internal:///./kotlin/ballast-ballast-api.js:3576:88)
    at protoOf.enqueueQueued_vbcpz3_k$ (webpack-internal:///./kotlin/ballast-ballast-api.js:3865:16)
    at protoOf.sendToQueue_bkzj57_k$ (webpack-internal:///./kotlin/ballast-ballast-api.js:5405:30)
    at protoOf.doResume_5yljmg_k$ (webpack-internal:///./kotlin/ballast-ballast-utils.js:230:48)
    at protoOf.resumeWith_b9cu3x_k$ (webpack-internal:///./kotlin/kotlin-kotlin-stdlib.js:6846:34)

[rocketraman]

@Casey Brooks Figured it out -- my vm configs are mostly singletons that come from my DI system. I don't know if this is the intended behavior or not for ballast, but having the configs be singletons did work fine in 2.x. However, in 4.x it causes the exception seen above. I've created a repro for you here: https://github.com/rocketraman/test-ballast-4-csce.

[Casey Brooks]

The intention has always been that the VM configs and config builders are not reused between VMs. Even the same VM type which creates multiple instances should still create new configs. Your DI setup should manage them as factories, not singletons.

This page in the docs does show those builders be provided as factories rather than singleton, but it’s definitely not very clear. I will work on updating the docs to clarify that.

[rocketraman]

Yep, I made the change to factory as soon as I figured out what was happening, but it was a bit surprising in the sense that "configs" are not normally tied to lifecycles.

[Casey Brooks]

The config is basically an ephemeral class that is used to populate the VM, essentially just a wrapper class to avoid a huge constructor. It doesn’t really live beyond the creation of the VM, but it does contain values that are directly related to the lifecycle of the VM and may hold state related to the VM instance (InputStragegy, interceptors, etc.). You should always treat it as a factory to ensure that those values are recreated when the VM itself is recreated.

But I can definitely see the confusion here. The docs really only show the usage of the builder being applied directly to the VM’s constructor, and doesn’t really explain much about the config class itself

[rocketraman]

If that is so, the API really should be changed to just accept config parameters in the constructor. That way there is no way for the caller to mess things up by having the lifecycle of the config be different from the lifecycle of the VM.

[rocketraman]

Or alternatively, the VM implementation should think of configs as templates from which it can make copies of things it needs with its own lifecycle (this would be my preference).

[Casey Brooks]

That’s basically how the config builder has been treated, so it seem like that’s basically just duplication of the construction logic, not to mention a ton of work to update all the existing classes/interceptors to themselves be factories rather than concrete instances. But I do see, now, how that would help ensure it cannot be mis-configured

But I also don’t feel like this is necessarily an unusual pattern the way it is. Take Ktor HTTP clients, for example. It also uses a pattern of a builder DSL which creates a Config class that’s tied directly to the HTTP client. Granted HTTP clients themselves are usually singletons, and maybe that’s the distinction and confusion here.

[rocketraman]

> Granted HTTP clients themselves are usually singletons, and maybe that’s the distinction and confusion here.
Yep, that's right.

[rocketraman]

Also, the failure mode was a bit nasty. It took me quite a few hours to debug why it wasn't working.

[rocketraman]

I can't think of an example of a popular API in which an object which is intended to be created each time it is used, uses a configuration DSL which is also tied to the same lifecycle. Config is generally config -- not state.

[rocketraman]

The other option is to take the builder itself as the parameter to the constructor.

[Casey Brooks]

I don’t think that would fix the problem though, since the builder doesn’t create new instances of the classes which hold state. I think it would need to be updated so that Interceptors and the other stateful classes themselves are factories, similar to how they’re declared in Ktor.

I don’t have a ton of time right now (through the end of the year) for such a major change, but if this is something you’d like to poke around at, I’m definitely open to that contribution!

[rocketraman]

You're right, just tried it, and using the builder as a builder on each construction of the VM doesn't work either. That's even worse from an API affordance perspective :-)

[Casey Brooks]

Yeah, again I wasn’t considering the use case of creating the config separately from the VM instance. I was mostly thinking of it as little more than a big, mutable constructor. The only real difference between the builder and the actual config class is var vs val properties

[rocketraman]

I'll create an issue so at least future people that run into this and google for it might find the solution quickly, and perhaps over time someone can adjust the API or implementation

[rocketraman]

And here it is: https://github.com/copper-leaf/ballast/issues/62

[rocketraman]

Maybe another option for an API change that would not introduce drastic changes, but would make the API affordances clearer, would be take a lambda parameter configCreator: () -> BallastViewModelConfiguration<Inputs, Events, State> in the VM constructor. The intended usage is then obvious to the caller, and for current API users this would be an easy change to make. It can be introduced into 4.0 by deprecating the old constructor, and then removing the old method in 5.x.