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

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

Closed
muhuk opened this issue Aug 14, 2015 · 9 comments
Closed

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

muhuk opened this issue Aug 14, 2015 · 9 comments

Comments

@muhuk
Copy link

muhuk commented Aug 14, 2015

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
Copy link
Author

muhuk commented Aug 14, 2015

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

@muhuk
Copy link
Author

muhuk commented Aug 14, 2015

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 muhuk closed this as completed Aug 14, 2015
@muhuk
Copy link
Author

muhuk commented Aug 15, 2015

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
Copy link
Collaborator

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

@muhuk
Copy link
Author

muhuk commented Aug 17, 2015

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
Copy link
Collaborator

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
#47 (comment).

@muhuk
Copy link
Author

muhuk commented Aug 17, 2015

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
Copy link
Collaborator

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
Copy link
Author

muhuk commented Aug 18, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants