Cancelling reactimates

[PaulJohnson]

This is a proposed fix for #198. The Reactive.Banana.Frameworks is extended with the following:

reactimate1 :: Event (IO ()) -> MomentIO (IO ())`

Like "reactimate", but this returns an IO action which will cancel the event actions.

This is useful when an event passed to "execute" calls "reactimate". For instance if the event creates a GUI widget which is then updated using "reactimate" then the updates will carry on being executed even if the widget is no longer displayed. By making the result of this function into a callback for widget destruction you can ensure that the widget updates stop once they are no longer required.

If the cancellation function is called from within "execute" then it will block. Should this occur then fork the cancellation off as a separate thread using Control.Concurrent.forkIO:

stop <- reactimate1 myEvent onUpdateNotNeeded $ forkIO stop

However in this case there is no guarantee that the event will be cancelled immediately.

And likewise there is a reactimate1' for future values.

I'm not entirely happy with the way that the cancel operation works: it calls "runStep", so if it gets called from inside runStep (which I think happens during execute) then the program locks up. The solution, as mentioned above, is to wrap the cancel action in forkIO. I tried doing this inside reactimate1 but that led to non-deterministic behaviour in the tests.

If there is a better way to implement this extension then I'd be happy. Having the cancellation action be of type MomentIO () instead of IO () would do this, but then I wouldn't be able to use it from inside a widget call-back. Of course I could set up an event to send these to a reactimate, but that seemed overkill.

[ocharles]

@HeinrichApfelmus it would be great to have your thoughts on this one.

[HeinrichApfelmus]

Thank you very much for taking the time to look into this!

A.

If there is a better way to implement this extension then I'd be happy. Having the cancellation action be of type MomentIO () instead of IO () would do this, but then I wouldn't be able to use it from inside a widget call-back. Of course I could set up an event to send these to a reactimate, but that seemed overkill.

Yes, I actually prefer the type with MomentIO. To use it from inside a widget callback, I'd be happy to see two additional functions

runMomentIO  :: EventNetwork -> MomentIO a  -> IO a
runMomentIO_ :: EventNetwork -> MomentIO a  -> IO ()

that essentially use execute on the MomentIO. It does the same thing that your runStep construction currently does. I wanted this function for unrelated reasons anyway, so feel free to implement it. If you implement both, please only export the second one for the time being.

B.

I'm not happy with the way that the Output is currently created, with the argument being error in most cases. Can you at least change the Output data type so that _evalO field becomes a Maybe? A better solution would be to make removeReactimate do nothing until the pulse has actually been created, but this is difficult to synchronize within this buildLater construction. I may have to think more about this.

C. The name reactimate1 is unusual. Ideally, the two functions should be called

reactimate  :: Event (IO ()) -> MomentIO (IO ())
reactimate_ :: Event (IO ()) -> MomentIO ()

following the conventions from Control.Monad. Of course, this would break existing code horribly. Can you introduce reactimate_ as well with a small TODO note in the source code, so that we do not forget about this? At some point, we can switch the type of reactimate and deprecate the other function. (I would like to rename Event to the plural Events as well, see #147, but name changes should come in a single batch.)

Other than that, it looks pretty good to me! Thanks again!

[PaulJohnson]

A. runMomentIO_ has been added to Frameworks. I don't think its possible to create runMomentIO because it has to call runStep.

B. I agree that this is a kluge, but short of major surgery on the OrderedBag Output type I can't see a better way. What we actually need is to create the Unique tag which will be attached to the IORef in the Output and then pass that down into the buildLater call. However the OrderedBag can't delete an item based on the tag alone; it has to be attached to an IORef. Hence the dummy value.

Making the type a Maybe would add an extra level of indirection. Also it should not be possible for the value to remain unassigned after the call to addHandler, so if a Nothing value did somehow escape you would be stuck with either masking the bug or triggering an exception. The current code already does the latter, which I think is the Right Thing.

C. As requested. This means we also have reactimate_', which strikes me as a bad name. But short of breaking existing code (and I agree with your thinking on that) I don't see a better one.

[ocharles]

I'm personally still not set on this one. To me, cancelling a reactimate should happen by an Event being garbage collected. I sholudn't need to think about cancelling things, just ensuring that an Event is provably never.

[PaulJohnson]

I share your distaste, but I can't see how to make the GC understand that a reactimate is no longer needed.

My use case involves dynamic changes to a UI. When event Foo happens I need to execute code which sets up a bunch of widgets, including calling reactimate to link them to Bar events. Then when the Foo event happens again the widgets get thrown away and rebuilt from scratch. The new widgets are also linked to Bar, so those events keep on happening. Meantime the reactimate links for Bar to the old widgets are still there, so with every Bar event those old reactimate callbacks get executed, and they also stop the widgets themselves being GC'd. As more Foo events get executed these old widgets build up, creating a space time leak.

It might be possible to create some kind of cut-out primitive further back, so that Bar events going to the defunct widgets only have to be stopped once instead of for every reactimate call. However that doesn't change the fundamental nature of the problem: the programmer knows that the widgets are dead, but Reactive Banana doesn't.

Using whenE could stop the events, but not provably: the old widgets still wouldn't be GCd.

PS. This PR is now so bit-rotted it's going to be easier to redo from scratch. I expect to create a new PR in the next day or two.

[ocharles]

I think we need to create a minimal example of the problem, and then we can start to iterate on a solution. I think what you're saying is we start with something like:

f = do
  execute $ e1 $> do
    reactimate $ f <$> e2

This means whenever e1 occurs, we'll execute and attach a new reactimate. This is the "leak" - as e1 continues to occur, we only ever create new reactimates. What we actually want is that whenever e1 occurs any old reactimates are cancelled, and only the new reactimate is added. I suggest we do this as:

f = do
  execute $ e1 $> do
    e2' <- e2 `untilNext` e1
    reactimate $ f <$> e2'

untilNext is the missing piece of the puzzle here. We could implement untilNext with whenE, but as you say that's going to create a space leak. What we need is a version of untilNext that returns never once the "until" Event fires. I think we can do this with the new switchE (for example, we just did this for once)

untilNext :: MonadMoment m => Event a -> Event b -> m Event a
untilNext e1 e2 = do
  nextE2 <- once e2
  switchE e1 (never <$ nextE2)

We'll need to thoroughly test such a combinator, but hopefully this motivates my intentions.

The other bit of work is to make sure that reactimate is actually garbage-collected if its driving event becomes never.

[PaulJohnson]

Yes, that looks like a better way.

A skeleton example would be something like:

 makeDialog :: ConfigData -> Event FieldData -> MomentIO ()
 makeDialog config fdEvent = do
    widget <- createWidgets config
    reactimate $ (updateWidgets widget) <$> fdEvent

outer :: Event ConfigData -> Event FieldData -> MomentIO ()
outer configEvent fdEvent = do
   void $ execute $ configEvent <&> \cd -> makeDialog cd fdEvent

With your untilNext primitive that would become

outer configEvent fdEvent = do
   void $ execute $ configEvent <&> \cd -> do
      fdEvent' <- fdEvent `untilNext` configEvent
      makeDialog cd fdEvent'

[PaulJohnson]

Here is a simple demo of the problem
banana-leak-0.1.0.0.tar.gz
.

[ocharles]

Awesome, thanks @PaulJohnson! I'll see what I can do. I'm also keen to have a solution here.

[HeinrichApfelmus]

Keep me posted as well. If there's one thing that I'd like to get straight in reactive-banana, it's garbage collection.

Essentially, the main tension is this: In Ollie's example, e2 could be garbage collected if no reactimate were not listening to it. On the other hand, if e2 is never, then the reactimate can be garbage collected. More prosaically: A tree can be garbage collected if there is no one to hear it fall, but also if it depends on a tree that has fallen long ago.