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

Fix memory management gotcha #33

Closed
raquo opened this issue Aug 17, 2018 · 3 comments
Closed

Fix memory management gotcha #33

raquo opened this issue Aug 17, 2018 · 3 comments

Comments

@raquo
Copy link
Owner

raquo commented Aug 17, 2018

Current problematic behaviour described in the docs:

Every Laminar element is an Owner. An element kills the resources that it owns when that element is discarded. An element is discarded when it is removed from the DOM. Now, here is an unfortunate loophole: what about a Laminar element that was created, owns some State or subscriptions, but that was never inserted into the DOM? Since it was never inserted, it will never be removed from it, so the resources that it owns will never be cleaned up. Unfortunately there is currently no way around this. This is a design bug that I will fix in a later version. So for now, do not proactively create elements that will never be added to the DOM. The most trivial workaround is to create a factory that creates an element when it's actually needed instead.

I plan to address this by starting the element's subscriptions when the element is inserted into the DOM rather than when the element is created. That way, the elements that were created but never inserted into the DOM will not hold any leaky resources, and so can be discarded without worrying about the subscriptions.

This means that an element that has subscriptions will need to listen to its own mountEvents stream, but it already does that for the purpose of unsubscribing when the element gets unmounted (see ReactiveElement.onOwned).

This solution will fix the issues when using ReactiveElement's subscribe* methods. However, as elements are Owners, they can also be used directly in methods where an implicit owner is required, which would still not be safe for the reason described above. Perhaps we need to avoid exposing elements as Owners, or maybe even extend Airstream's ownership API to allow for a delayed subscription start (basically, bring the subscribe* methods into the Owner?). This might also indirectly help us deal with the other memory management gotcha:

When a Laminar element is removed from the DOM, the resources that it owns are killed with no built-in way to resurrect them.

However, this starts looking a lot like the dynamic subscriptions system we had in previous versions of Laminar, where subscriptions could be turned on and off as the element gets mounted and unmounted. We should investigate if we can bring back this system without falling into other kinds of memory leaks (esp. zombie references).

@raquo
Copy link
Owner Author

raquo commented Oct 20, 2018

Well, damn, this is indeed really hard.

Let's summarize the problem once again: when we create a Laminar element like so:

val el = div(onClick --> clickStream, child.text <-- textState)

we immediately create the required subscriptions for clickStream and textState. We will kill these subscriptions when el becomes unmounted, except this line of code does not actually mount it into the DOM, it merely creates the element. So it is entirely possible that el will never be mounted, and therefore that it will never be unmounted, and its subscriptions will live on forever, screwing up our logic and causing memory leaks.

The solution I was hoping to get working is similar to DynamicSubscription feature of Laminar v0.2 – basically, don't start the subscriptions until the element is mounted. Sounds simple enough – we can now safely discard el without mounting it. However, there's a deeper, nastier problem with this. Laminar elements are Owner-s, and we need those not just for subscriptions, but also for creating state variables such as textState. Indeed, it is easy to imagine a method like this:

def makeDiv(textSignal: Signal[String]): Div = {
  val el = div("Number of people:")
  val textState = textSignal.toState(owner = el) // imagine there's a reason to make it a State
  el <-- child.text <-- textState
  el
}

Same situation as before – simply calling that method and ignoring its output is enough to cause a memory leak (textState will never stop, and thus textSignal won't, either).

However, observe how we made el the Owner of textState. State gives us certain guarantees – namely, that once instantiated it will always run until its owner tells it to permanently shut down. The proposed DynamicSubscription would be incompatible with this guarantee, making State effectively indistinguishable from the lazy Signal.

Basically, to solve this memory management issue for good, we can't have Laminar elements remain Owners. Perhaps this calls for introduction of some kind of Component type, although I'm not quite sure what role it would play other than somehow provide an owner and maybe manage element lifecycle (given that this is what tracks mounting / unmounting).

This could potentially improve performance too, as we would only need to keep track of Components' root elements mounted status. I do not want to give up Laminar's compositional freedom. Currently you do not need to wrap your code in any wrappers, and can freely use functions / classes / objects / whatever to build your components. React's notion of components does not suit me at all, it's too constrained.

Less radically, I think perhaps we could introduce a Fragment[El <: ReactiveElement] type like Owner => El. We would recommend using this type for any reactive elements that you aren't 100% sure will be eventually mounted. We wouldn't be able to enforce it, but it is a relatively easy rule of thumb to follow, similar to don't pass State to a different owner's context, pass Signals instead that we already have.

Another option could be to provide something of a whenMounted hook, like so:

val el: Div = div.whenMounted { (owner, thisNode) =>
  val textState = textSignal.toState(owner) // imagine there's a reason to make it a State
  List(
    "Hey, "child.text <-- textState
  )
}

This seems like what people would generally intend state to work like, however this makes it hard to make a component that returns not just an element but also other data like perhaps some relevant streams that are generated from its state. So, this actually sounds like a use case for a well designed Component type. Hrmmmm.

I need to sleep on this... repeatedly.

@raquo raquo modified the milestones: v0.4, v0.5, v0.6 Oct 20, 2018
@raquo
Copy link
Owner Author

raquo commented Oct 21, 2018

The Component / Fragment approach looks suspiciously like IO :|

Kinda want to avoid going in that direction, having everything wrapped in a monadic type like that.

@raquo
Copy link
Owner Author

raquo commented Oct 22, 2018

A few more things for my own clarity:

  • <-- and --> work just fine in terms of memory management, if we switch to a model where we start those subscriptions when the relevant element is mounted (as opposed to when it's created)
  • It's the "component's" state that we have a problem with. Ideally, elements should not be owners, they should contain a private owner and expose safe hooks to use it.
  • <-- and --> are some of those, but I think we need another one that would let you define Owner => A, where the Owner param is provided by the element, and A is an arbitrary output type.
  • A needs to be arbitrary in order to preserve easy composition. Perhaps it should be (owner, thisNode) => A instead, so that we can return thisNode is this is what we need. But we should also be able to return other things like bundles of nodes and streams.
  • This still feels too restrictive, however. If we go this way, a component will still need to be defined by a single element. Perhaps a Component type wrapping a Owner => A is better still, but it would require an Owner that is not related to a particular Element. The Component would need to be a Mod, I guess, similar to how an element is a Mod. The component would then be able to subscribe to its parent's mount events, so it would not become a toxic type like IO.
  • If we switch to subscriptions activating / deactivating onMount, we will need to provide convenience methods around that. If you unmount a component in React, all its state is lost. In Laminar, we can choose what we want to do.
  • None of this forces us to solve re-mounting unmounted elements. Component could potentially discard and re-run all of its contents when it's mounted again to get a clear state. But should it? I don't think so. This is all too abstract right now.

@raquo raquo modified the milestones: v0.6, v0.7 Dec 30, 2018
@raquo raquo added need to find time https://falseknees.com/297.html and removed discussion labels Feb 4, 2019
@raquo raquo modified the milestones: v0.7, v0.8 Apr 17, 2019
@raquo raquo removed the need to find time https://falseknees.com/297.html label Feb 2, 2020
raquo added a commit that referenced this issue Feb 8, 2020
- Use new DynamicOwner from Airstream
- Element subscriptions are only activated on element mount, not on element init
- This fixes the main memory management gotcha: Fixes #33
- New pilot subscription fixes #47
@raquo raquo closed this as completed in 8dec49c Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant