Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

solatis
Copy link

@solatis solatis commented Mar 22, 2020

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:


(def control-chan (md/deferred))
(def x (md/deferred))

(def y (alt control-chan x))
(md/success! x {:foo :bar})

(println "control chan listeners: " (pr-str (.-listeners control-chan))))

You will see that, even though both x and y have realized, the listener on control-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.

Comment on lines +1122 to +1125
(fn [&args]
(cancel-listener! x l))
(fn [&args]
(cancel-listener! x l)))
Copy link
Collaborator

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.

Copy link
Collaborator

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.

@KingMob
Copy link
Collaborator

KingMob commented Jan 11, 2021

@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.

@KingMob
Copy link
Collaborator

KingMob commented Jan 11, 2021

@ztellman Does this analysis make sense to you?

@solatis solatis requested a review from slipset as a code owner August 30, 2021 03:41
@solatis
Copy link
Author

solatis commented Sep 11, 2021

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.

@KingMob
Copy link
Collaborator

KingMob commented Sep 27, 2021

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.

@KingMob KingMob self-assigned this Oct 9, 2021
@KingMob
Copy link
Collaborator

KingMob commented Oct 9, 2021

@solatis Any time to work on this?

@KingMob KingMob requested review from KingMob and removed request for slipset April 26, 2022 07:38
@KingMob KingMob added the waiting on user Can't proceed without user input or info label Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on user Can't proceed without user input or info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants