reactive-banana
reactive-banana copied to clipboard
Register handlers on actuate and unregister them on pause
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
Is there anything I can do to help move this along? This PR has been sitting here untouched for quite a while now.
Unfortunately I don't think reactive-banana
is really under active development at the moment.
@Kritzefitz Are you still interested in getting this one through?
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).
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.
I just pushed a new commit with your suggestions applied. As you suggested, the registerHandler
s are now run from right to left (earliest added to latest added), but opposed to that unregisterHandler
s are run from left to right (latest registered to earliest registered). I think that makes sense to allow unregisterHandler
s to reference resources initialized by another earlier registerHandler
before it is cleaned up by that registerHandler
's unregisterHandler
.
LGTM! Thanks! =)
Two last things -
- @HeinrichApfelmus did you want to look this over before merging?
- Could you add an entry to the changelog and your name to the list of contributors, if it isn't there already?
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.
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
andpause
never callfromAddHandler
. -
runStep
might callfromAddHandler
throughexecute
. However the network state is only locked while evaluating the condition forwhenM
. So no chance of collision here. -
fromAddHandler
might call all three of those functions when the user supplied handler calls eitherpause
oractuate
directly orrunStep
through the passedfire
callback and evenfromAddHandler
throughfire
andexecute
.-
actuate
andpause
now deadlock when called from insidefromAddHandler
, because they try to touch the network state when it is already held byfromAddHandler
. -
runStep
when called throughfire
deadlocks when called fromfromAddHandler
inexecute
. This was already the case before my changes, becauserunStep
holds a lock ons
during calculation of the nextstep
. -
fromAddHandler
can not be called inside anotherfromAddHandler
because the only way to do so, would be to call it through anotherrunStep
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 toActuating
-
PausedWhileRegistering
-> set state toRegistering
-
Pausing
-> set state toActuatedWhilePausing
-
Paused
- Set state to
Actuating
- run
AddHandlers
- check state again
-
Actuating
-> set stateActuated
-
PausedWhileActuating
-> callpause'
thus transitioning toPausing
- everything else -> error
-
- Set state to
-
-
pause
-
Paused
,Pausing
,PausedWhileActuating
,PausedWhileRegistering
-> noop -
Actuating
-> set statePausedWhileActuating
-
ActuatedWhilePausing
-> set statePausing
-
Registering
-> set statePausedWhileRegistering
-
Actuated
- Set state
Pausing
- unregister handlers
- check state again
-
Pausing
-> set statePaused
-
ActuatedWhilePausing
-> callactuate'
thus transitioning toActuating
- everything else -> error
-
- Set state
-
-
runStep
-
Actuated
-> run step -
Paused
-> noop - everything else -> retry
-
-
fromAddHandler
-
Paused
-> recordAddHandler
in state -
Actuated
- Set state to
Registering
- call new
AddHandler
- check state again
-
Registering
-> Set stateActuated
with new handlers -
PausedWhileRegistering
-> callpause'
with new unregisterer thus transitioning toPausing
-
- Set state to
-
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.