Register handlers on actuate and unregister them on pause

[Kritzefitz]

This should fix the issue that handlers registered by fromAddHandler were not unregistered by pause. It also brings the implementation in line with the documentation, which says that input handlers will only be registered once actuate is called.

Closes #73

[Kritzefitz]

Is there anything I can do to help move this along? This PR has been sitting here untouched for quite a while now.

[ocharles]

Unfortunately I don't think reactive-banana is really under active development at the moment.

[mitchellwrosen]

@Kritzefitz Are you still interested in getting this one through?

[mitchellwrosen]

I've read over the implementation, and I think it looks 👍 overall, but I did find the encoding of the state of the network rather confusing to puzzle through. I have a general suggestion for a refactoring that I think could improve things.

This patch encodes the state of the event network (actuated/paused), the "register" actions we've accumulated (either while actuated or paused), and the "unregister" actions to call when paused all in the same "anonymous" data type:

([IO (IO ())], Maybe [IO ()])

I suggest perhaps writing down a dedicated data type, such as:

data NetworkState
  = NetworkState
   { actuated :: Bool
   , registerHandlers :: [IO (IO ())]
   , unregisterHandlers :: [IO ()]
   }

Second, I think everywhere you've written takeMVar / putMVar, a withMVar would be preferred.

And finally, since we (:) new handlers onto these lists, we should probably be careful to run the actions in the reverse order on both actuate and pause. (This NetworkState encoding also fixes the issue where the unregisterHandlers list doesn't actually exist while the network is paused, which means something complicated would have to happen to respect the original fromAddHandler ordering here, since you can call fromAddHandler while the network is actuated or paused).

[Kritzefitz]

Generally I'd be willing to make those adjustment. The more interesting question is when I will have the time to do it, as I can't really make a commitment in that regard right now.

[Kritzefitz]

I just pushed a new commit with your suggestions applied. As you suggested, the registerHandlers are now run from right to left (earliest added to latest added), but opposed to that unregisterHandlers are run from left to right (latest registered to earliest registered). I think that makes sense to allow unregisterHandlers to reference resources initialized by another earlier registerHandler before it is cleaned up by that registerHandler's unregisterHandler.

[mitchellwrosen]

LGTM! Thanks! =)

Two last things -

1. @HeinrichApfelmus did you want to look this over before merging?
2. Could you add an entry to the changelog and your name to the list of contributors, if it isn't there already?

[HeinrichApfelmus]

Thanks a lot for your work!

However, I actually do want to look over it. The issue is that even though the code looks fine, I'm not entirely certain that it does the right thing when new inputs are added in the context of say, execute; I have the feeling that such usage might result in a deadlock (Does the BarTab.hs example in reactive-banana-wx still work?). In comparison, using reactimate adds outputs in a "delayed" manner. I need to mull over this a bit.

[Kritzefitz]

Could you add an entry to the changelog and your name to the list of contributors, if it isn't there already?

Done

The issue is that even though the code looks fine, I'm not entirely certain that it does the right thing when new inputs are added in the context of say, execute; I have the feeling that such usage might result in a deadlock (Does the BarTab.hs example in reactive-banana-wx still work?). In comparison, using reactimate adds outputs in a "delayed" manner. I need to mull over this a bit.

I couldn't get reactive-banana-wx to compile, because wx doesn't seem to compile with newer GHC versions, while old enough GHC versions panic when used on my current setup. However, I tried my modified version using this commit from my own application. It does use execute to dynamically create new inputs at runtime and I observed no adverse effects. But that might not cover all use cases you have in mind.

I'll try to write down my theoretical thoughts about this: There is added potential for deadlocks, because the network state MVar (previously just the boolean actuated) is now touched in fromAddHandler in addition to previously just actuate, pause and runStep. New deadlocks can arise when fromAddHandler calls one of those other three functions while holding the lock or vice versa.

• actuate and pause never call fromAddHandler.
• runStep might call fromAddHandler through execute. However the network state is only locked while evaluating the condition for whenM. So no chance of collision here.
• fromAddHandler might call all three of those functions when the user supplied handler calls either pause or actuate directly or runStep through the passed fire callback and even fromAddHandler through fire and execute.
  • actuate and pause now deadlock when called from inside fromAddHandler, because they try to touch the network state when it is already held by fromAddHandler.
  • runStep when called through fire deadlocks when called from fromAddHandler in execute. This was already the case before my changes, because runStep holds a lock on s during calculation of the next step.
  • fromAddHandler can not be called inside another fromAddHandler because the only way to do so, would be to call it through another runStep which already deadlocks for other reasons as explained above.

I'm not sure if there are use cases for calling actuate or pause from fromAddHandler. Calling actuate from an AddHandler is always a noop, because the network has to be actuated for the AddHandler to be executed in the first place. The semantics of calling pause from inside an AddHandler seem kinda wonky to me, as that would mean imediately unregistering the handler we are in the middle of registering.

If we want to support those cases, we probably need much more explicit locking for the network state, such as:

data NetworkState
  = Actuated { registerHandlers :: [IO (IO ())], unregisterHandlers [IO ()] }
  | Actuating
  | PausedWhileActuating
  | Paused { registerHandlers :: [IO (IO ())] }
  | Pausing
  | ActuatedWhilePausing
  | Registering
  | PausedWhileRegistering

The state would then have to be kept in a TVar and the functions handlings the state would work as follows:

• actuate
  • Actuated, Actuating, Registering, ActuatedWhilePausing -> noop
  • PausedWhileActuating -> set state to Actuating
  • PausedWhileRegistering -> set state to Registering
  • Pausing -> set state to ActuatedWhilePausing
  • Paused
    1. Set state to Actuating
    2. run AddHandlers
    3. check state again
       • Actuating -> set state Actuated
       • PausedWhileActuating -> call pause' thus transitioning to Pausing
       • everything else -> error
• pause
  • Paused, Pausing, PausedWhileActuating, PausedWhileRegistering -> noop
  • Actuating -> set state PausedWhileActuating
  • ActuatedWhilePausing -> set state Pausing
  • Registering -> set state PausedWhileRegistering
  • Actuated
    1. Set state Pausing
    2. unregister handlers
    3. check state again
       • Pausing -> set state Paused
       • ActuatedWhilePausing -> call actuate' thus transitioning to Actuating
       • everything else -> error
• runStep
  • Actuated -> run step
  • Paused -> noop
  • everything else -> retry
• fromAddHandler
  • Paused -> record AddHandler in state
  • Actuated
    1. Set state to Registering
    2. call new AddHandler
    3. check state again
       • Registering -> Set state Actuated with new handlers
       • PausedWhileRegistering -> call pause' with new unregisterer thus transitioning to Pausing

actuate' and pause' would then be variants of actuate and pause that receive an Actuated or Paused state from the caller and transition the global state to Actuating or Pausing regardless of the previous state, under the assumption that the caller made sure that the resulting state transition is correct.

With the rules above, actuate and pause might return before the network is in the requested state, as long as another thread is working to achieve the desired state. Alternatively the transition state (Actuating, PausedWhileActuating, Pausing, ActuatedWhilePausing, Registering, PausedWhileRegistering) could record which thread is processing that state transition. actuate and pause could then block on those states, when another thread is processing the transition. They would still need to quietly return without a real state change when they are in the thread that is supposed to process that transition to avoid deadlocking the transition.