-
Notifications
You must be signed in to change notification settings - Fork 107
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 issue with listeners not being cleaned up in alt' #188
base: master
Are you sure you want to change the base?
Conversation
(fn [&args] | ||
(cancel-listener! x l)) | ||
(fn [&args] | ||
(cancel-listener! x l))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@solatis I would avoid doing [&args]
here, since that's actually a valid symbol name. This will break if more than one arg is passed. Try something like [& _]
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if I'm not mistaken, a listener can be shared, so you can move its creation outside the loop and reuse it for each alternative.
@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 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 |
@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 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.