clj-commons / manifold

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

let-flow chokes when the body closes over a deferred. #47

Closed muhuk closed 9 years ago

muhuk commented 9 years ago

These two should be identical:

(ns let-flows-not
  (:require [manifold.deferred :as d]
            [manifold.stream :as s]))

(let [k (d/deferred)
      q (s/stream)
      x (d/chain' (s/try-put! q :x 10)
                  (fn [_] 
                    k
                    {:result :chain}))]
  (Thread/sleep 20)
  (prn :debug :x (d/realized? x))
  (when (d/realized? x)
    (prn :debug :x @x)))

(let [k (d/deferred)
      q (s/stream)
      x (d/let-flow' [_ (s/try-put! q :y 10)]
                     k
                     {:result :let-flow})]
  (Thread/sleep 20)
  (prn :debug :x (d/realized? x))
  (when (d/realized? x)
    (prn :debug :x @x)))

But using [manifold "0.1.1-alpha3"] the output is:

:debug :x true
:debug :x {:result :chain}
:debug :x false

It seems the second let is blocking because k is not realized.

muhuk commented 9 years ago

@ztellman same code, I've just added k's to each.

muhuk commented 9 years ago

It seems let-flow is doing more than I had imagined. Looking at this test case:

(is (= 5
        @(let [z (clojure.core/future 1)]
           (d/let-flow [x (d/future (clojure.core/future z))
                        y (d/future (+ z x))]
             (d/future (+ x x y z))))))

y depends on z, so blocking on z is alright here. But vars' (and in return deps?) is capturing everything, so everything blocks. I understand it's easy to implement this way, but (in my 1st example above) blocking on deferreds we don't act on is unnecessary.

Now that I've checked the docs again, it clearly says; "This is only true of values declared within or closed over by let-flow, however.". So it looks like a RTFM case, I'm closing this ticket.

muhuk commented 9 years ago

I have created alternate versions that don't block on everything: modest-let-flow

I'd be happy to send a PR if we can come up with some descriptive name like let-flow*'$!.

ztellman commented 9 years ago

So in your code, you didn't care about the closed over value being realized? Why were you referencing it, in that case?

muhuk commented 9 years ago

Consider this code:

(let [x (d/deferred)]
  (d/let-flow [y (fn-returns-deferred)]
              (if y
                x
                :sumtin)))

It would block on x, but perhaps if y is false we will never realize x.

In my case x goes into a stream and y is the result of try-put!. Only if put succeeds can x be realized. So it's not so much not caring, let-flow simply doesn't work in my case.

ztellman commented 9 years ago

Interesting, the answer here may be to just make let-flow more ambitious, and have it properly handle conditional branches. Thanks for pointing out this corner case, I'll give it some thought.

On Mon, Aug 17, 2015 at 10:21 AM Atamert Ölçgen notifications@github.com wrote:

Consider this code:

(let [x (d/deferred)](d/let-flow [y %28fn-returns-deferred%29] %28if y x :sumtin%29))

It would block on x, but perhaps if y is false we will never realize x.

In my case x goes into a stream and y is the result of try-put!. Only if put succeeds can x be realized. So it's not so much not caring, let-flow simply doesn't work in my case.

— Reply to this email directly or view it on GitHub https://github.com/ztellman/manifold/issues/47#issuecomment-131894787.

muhuk commented 9 years ago

make let-flow more ambitious, and have it properly handle conditional branches.

Or make it more ambitious and only block on deferreds referenced by the deferreds in bindings. It would still be a bit surprisey that:

;; somewhere-else
(future
  (+ 1 2 @x))

;; yet:
(let-flow [y (d/future (+1 2 x))] ...)

...but at least we'd know we can block on x safely.

In any case I ♥ Manifold.

ztellman commented 9 years ago

The "contract" of let-flow is that "any deferred value that is referenced can be treated as if it's a realized value". I would rather make that a fully realized guarantee rather than weaken it. Does that make sense, or do you feel it's more confusing than helpful?

muhuk commented 9 years ago

No I get it. That's why I closed the ticket.

I have two issues here (that I've solved for myself with the modest version):

  1. Branching breaks. You can perhaps analyze the code further and fix for simple cases. But what if my in the body I'm calling a function with some conditional?
  2. Too much magic. Consider a app/lib using Manifold and someone else is reading their code. If it's an @x they will get that it's some sort of deferred. If it's an x they will assume it's a value. Then later they will find out it's a deferred and they will wonder how does it even work. (Assuming they're not familiar with Manifold yet.)

All I need is syntactic sugar for let-zip-chain combo. I haven't found to a question where original let-flow behavior is the answer.

I'm not arguing or asking you to change anything. This is just how it looks from my point of view.