clj-commons / manifold

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

let-flow within loop blocks forever #187

Closed kennyjwilli closed 3 years ago

kennyjwilli commented 4 years ago

Running the below code will block the thread forever. The docs on deferred say "the combination of loop and let-flow can allow for the specification of highly intricate asynchronous workflows". That seems to imply that the below code should work.

(require '[manifold.deferred :as deferred])
(require '[aleph.http :as http])

(defn request-example
  [request-map]
  (deferred/loop
    []
    (deferred/let-flow
      [resp (http/request request-map)]
      resp)))

@(request-example
   {:method            :get
    :url               "https://httpstat.us/200"
    :request-timeout   1000
    :throw-exceptions? false})
Hamled commented 4 years ago

I've noticed this as well. On the aleph website there are examples for a TCP server which include equivalent implementations using chain and let-flow, both in a loop form (search for slow-echo-handler).

The implementation using chain works as indicated by the example code, but the implementation with let-flow blocks forever on taking a message from the client stream.

theronic commented 3 years ago

I spent a long time trying to understand what I was doing wrong, but md/let-flow does seem to block inside md/loop.

Using d/chain with d/loop and d/recur works, however.

E.g. the let-flow body here is never reached:

(let [s (ms/stream)]
    (ms/put! s 123)
    (md/loop []
      (->
        (md/let-flow [x (ms/take! s)] ;; here let-flow hangs forever, even when using ms/try-take! with a timeout.
          (log/debug "This does not print" x)
          (when x
            (md/recur)))
        (md/catch (fn [ex]
                    (log/error "Exception" ex))))))

@ztellman I've tried updating to aleph/aleph {:mvn/version "0.4.7-alpha7"}, but am seeing the same result. Am I misunderstanding something about how Manifold works?

ztellman commented 3 years ago

I'm sorry for the very slow reply on this, as you may have noticed my maintenance on all of my Clojure projects isn't very active these days. This is a pretty egregious one, though, since loop and let-flow are meant to be freely composable. The issue was that let-flow does analysis on the lexical scope outside of the let-flow form, and was incorrectly blocking on the deferred that loop had created to represent its eventual value, causing a deadlock. I'll cut a new release so that the fix can be used, and will continue to chip away at the backlog of issues, with an eye towards creating 1.0 releases for both Aleph and Manifold.

theronic commented 3 years ago

Thanks, @ztellman! That fixes it.

Have submitted a PR to include a deps.edn in Manifold to refer to a specific commit. Until a new release is deployed to Clojars, those who want to use this immediately without building locally, can temporarily refer to my fork in deps.edn:

aleph/manifold    {:git/url    "https://github.com/theronic/manifold.git"
                   :sha        "aec26d90c4c0eeb71394c416821d0c5fe8bb1fc9"}
ztellman commented 3 years ago

I pushed 0.1.9-alpha4, which has this change.

On Sun, Oct 25 2020 at 7:06 AM, Petrus Theron < notifications@github.com > wrote:

Thanks, @ztellman ( https://github.com/ztellman ) ! That fixes it.

Have submitted a PR ( https://github.com/aleph-io/manifold/pull/191 ) to include a deps.edn in Manifold to refer to a specific commit. Until a new release is deployed to Clojars, those who want to use this immediately without building locally, can temporarily refer to my fork in deps.edn:

aleph/manifold {:git/url "https://github.com/theronic/manifold.git"

:sha "aec26d90c4c0eeb71394c416821d0c5fe8bb1fc9"}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( https://github.com/aleph-io/manifold/issues/187#issuecomment-716153845 ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AAAHUP5IIETNFWOGARNQY3LSMQWGZANCNFSM4KNOHLOA ).