clj-commons / manifold

A compatibility layer for event-driven abstractions
1.02k stars 106 forks source link

Possible issue with alt #141

Open stoyle opened 6 years ago

stoyle commented 6 years ago

I am making a process which will run periodically, but it should also be possible to kill it. This is the code:

(defn foo [kill-d]
  (d/loop []
    (prn "Setting up with timeout" 1000)
    (d/let-flow [res (d/alt kill-d (d/timeout! (d/deferred) 1000 :ok))]
      (prn "Got a result" res)
      (if (= res :ok)
        (d/recur)
        (prn "Got a kill message for token, not recurring")))))

When running the alt does not trigger on the timeout.

(def kill (d/deferred))
=> #'user/kill
(foo kill)
"Setting up with timeout" 1000
=> << … >>
;; Witing a bit...

(d/success! kill 1)
"Got a result" 1
"Got a kill message for token, not recurring"

Modifying the code, removing the kill-d from the alt, now it works, except there is no way of killing it;

(defn foo [kill-d]
  (d/loop []
    (prn "Setting up with timeout" 1000)
    (d/let-flow [res (d/alt (d/timeout! (d/deferred) 1000 :ok))]
      (prn "Got a result" res)
      (if (= res :ok)
        (d/recur)
        (prn "Got a kill message for token, not recurring")))))

(def kill (d/deferred)) ; Not really necessary...
=> #'user/kill
(foo kill)
"Setting up with timeout" 1000
=> << … >>
"Got a result" :ok
"Setting up with timeout" 1000
"Got a result" :ok
"Setting up with timeout" 1000
"Got a result" :ok
"Setting up with timeout" 1000
"Got a result" :ok
"Setting up with timeout" 1000
"Got a result" :ok
"Setting up with timeout" 1000
"Got a result" :ok
"Setting up with timeout" 1000

I tried to simplify and it seems maybe timeout isn't the problem. Here alt only has to wait for two deferreds, and only seem to react on the last one:

(defn foo [d1 d2]
  (d/loop []
    (d/let-flow [res (d/alt d1 d2)]
      (prn "Got a result" res)
      (if (= res :ok)
        (d/recur)
        (prn "Got a kill message for token, not recurring")))))

=> #'user/foo
(def a (d/deferred))
=> #'user/a
(def b (d/deferred))
=> #'user/b
(foo a b)
=> << … >>
(d/success! a 1)
=> true
(d/success! b 1)
"Got a result" 1
"Got a kill message for token, not recurring"
=> true
dm3 commented 6 years ago

This seems to be a problem with loop/let-flow macro interaction and alt. If you convert the example to the explicit CPS form, the alt works fine. You can use this as an immediate fix for the problem:

(defn foo-chain [kill-d]
  (d/loop []
    (prn "Setting up with timeout" 1000)
    (-> (d/alt kill-d (d/timeout! (d/deferred) 1000 :ok))
        (d/chain
          (fn [res]
            (prn "Got a result" res)
            (if (= res :ok)
              (d/recur)
              (prn "Got a kill message for token, not recurring")))))))

I'll look into this closer a bit later.

dm3 commented 6 years ago

Here's a testcase:

(deftest alt-letflow
  (let [a (d/deferred), b (d/deferred), result (atom nil)]

    #_(d/let-flow [x (d/alt a b)]
      (reset! result x))

    (-> (d/alt a b)
        (d/chain #(reset! result %)))

    (d/success! a 1)
    (is (= 1 @result))

    (d/success! b 2)
    (is (= 1 @result))))
dm3 commented 6 years ago

So, this happens because let-flow transforms not only its own binding, but also picks up any locals in its lexical scope. In the macroexpansion of the test-case above, a and b get d/zipped together which defeats the purpose of alt.

I don't see a way to fix this without changing let-flow in complicated ways. Pinging @ztellman for an expert opinion :)

stoyle commented 6 years ago

In core.async it is idiomatic to use let with alt! and alts!. And I thought manifolds alt was a similar concept. I generally also think let-flow reads better than chain.

If it cannot work, for some reasons, I think it should be properly documented. Spent a bit of time trying to figure out why it wasn't working, and it was not at all obvious where the error was.