Open solatis opened 4 years ago
@solatis I've been diving into the code to try and understand your suggested PR.
I think there's a bug: the original code used chain'
, which internally calls unwrap'
. This will successively unwrap nested deferreds, but your code won't. Your listener fns will be called on the wrong value if a deferred is delivered to x
. It'll be passed the deferred, instead of continuing to unwrap.
E.g.:
(let [x (deferred)
l (listener #(println "listener success" %)
#(println "listener error" %))]
(on-realized (chain' x)
#(println "orig success" %)
#(println "orig error" %))
(add-listener! x l)
(success! x (success-deferred :foo)))
orig success :foo
listener success << :foo >>
I don't know if chain'
is needed here, but I definitely think `unwrap`` is.
@ztellman Does this analysis make sense to you?
Hi @KingMob I apologize I've been on vacation and busy with work and didn't notice your comments. Great to see things are moving forward, thanks a lot for that.
Regarding unwrap -- you're probably right. It doesn't hurt to add this anyway.
Not a problem. I'm still here.
One correction to my comment above about chain'
: I think you definitely to need to keep the (chain' x)
that's present in the original code. All we know about x
when your code runs is it's an IDeferred, but add-listener
can only be called on an IMutableDeferred. Wrapping it with (chain' x)
ensures that the listeners would have an IMutableDeferred to attach to.
I'm not seeing any inherent reason listeners couldn't be extended to promises/futures/delays and other non-IMutableDeferreds, but that's a bigger task.
@solatis Any time to work on this?
The current implementation of alt' has a bug where it doesn't clean up previous listeners once one of the deferreds have resolved.
This problem can be shown using the following code:
You will see that, even though both
x
andy
have realized, the listener oncontrol-chan
is still there. If you repeat this block of code many times, many listeners will remain active (we kept running into tens of thousands of runaway listeners).This PR addresses this problem, by using listeners instead. Upon realization of the deferred returned by
alt'
, it cancels all outstanding listeners.