Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config classes have lifecycles that are required to match the lifecycle of VMs #62

Open
rocketraman opened this issue Nov 13, 2023 · 0 comments

Comments

@rocketraman
Copy link

rocketraman commented Nov 13, 2023

VM config classes (and even config builder classes) are not independent in terms of lifecycle from the VMs constructed with them.

When this invariant is not held, then the system completely breaks. Adding logging shows that it throws ClosedSendChannelExceptions and then becomes unresponsive, as shown in the second reproduction in https://github.com/rocketraman/test-ballast-4-csce.

The workaround is simply to instantiate the config builder and config each time the VM is itself instantiated. In other words, when using a DI system, use a factory or provider rather than a singleton.

However, noting this issue because the API design allows for configs to be instantiated separately, but the implementation does not. This isn't the best API affordance for consumers, and should be "fixed" using one of the following solutions -- and I'm putting them in the order that I think is best to worst, from the perspective of an API consumer:

  1. The implementation of Ballast's VMs should be modified so that the lifecycle of a config is not tied to the lifecycle of the VM it is injected into.

  2. Modify the API in such a way that API consumers cannot do something so simple to break Ballast. The constructor of a VM could accept the config parameters directly, or it could accept a builder object which creates new instances of any stateful classes specified by the config at VM construction time.

  3. At the least, update documentation to explain the lifecycle of a config class, and update kdocs as well.

Reference conversation from Slack: https://kotlinlang.slack.com/archives/C03GTEJ9Y3E/p1699907007258999?thread_ts=1699623937.047949&cid=C03GTEJ9Y3E.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant