deferred/alt doesn't work right inside deferred/let-flow

[tanzoniteblack]

Trying be able to create a timeout value that I can share between multiple values, which is something with core.async I would write with multiple calls to alt!, all of which share the same timeout channel. But I'm finding that alt doesn't behave the same inside a let-flow and outside

(time (let [shared-timeout (d/timeout! (d/deferred) 1000 :timeout)]
        @(d/alt (d/future (Thread/sleep 500) "cat")
                   shared-timeout)))
"Elapsed time: 500.858473 msecs"
=> "cat"

but inside let-flow

(time (let [shared-timeout (d/timeout! (d/deferred) 1000 :timeout)]
        @(d/let-flow [x (d/alt (d/future (Thread/sleep 500) "cat")
                               shared-timeout)]
           x)))
"Elapsed time: 1001.190597 msecs"
=> :timeout

This issue was also pointed out as part of an older issue ( https://github.com/ztellman/manifold/issues/141 ), but since it's still a problem 2 years later, I wanted to open an issue explicitly for it with a minimal example.

[KingMob]

Hi @tanzoniteblack , would you be willing to backport your PR for this bug?

[tanzoniteblack]

@KingMob , this could probably be backported, but I'll also be up front that at Yummly we decided to avoid let-flow in our code base and instead use the tsasvla macro ( https://github.com/yummly/manifold/blob/master/docs/deferred.md#manifoldtsasvlatsasvla ) that we ended up creating using core.async's macros.

This bug fixed the timeout issue explicitly for alt, but we found we were playing whackamole by finding new core functions that needed updated (alt', zip, etc.). Additionally, we found a handful of other bugs with let-flow that ended up with us just throwing our hands up and saying let-flow wasn't worth it, as we were finding too many bugs with it & couldn't grok the code clear enough to convince ourselves there weren't other nefarious ones lurking around ( see https://github.com/yummly/manifold/pull/2 & https://github.com/yummly/manifold/pull/3 for two others we addressed before moving away from let-flow )

[KingMob]

Thanks, @tanzoniteblack . I may backport it myself when I get the time, (and the other two, if you don't mind), and then consider whether to mark let-flow as deprecated, discourage its use, or just attempt to articulate its constraints better in the docs.

[tanzoniteblack]

@KingMob , I have some time to do the backports this week. I've also created a PR for the tsasvla macro that I mentioned ( https://github.com/clj-commons/manifold/pull/200 ), which I've found to be a better solution overall for how to work with deferrables in a simple fashion.

[KingMob]

Thanks @tanzoniteblack . The tsasvla macro looks interesting, and core.async's lack of an error model has always really annoyed me. I'd be inclined to change the name back to the English "go", though, unless you know of good reasons not to. Ditto for aligning the name of <!-no-throw back to <!. Or are they different enough that this would lead to confusion? I notice there's no tsasvla put!/>! equivalent.

@ztellman makes a good point that we should mimic shadowing if possible in let-flow, but that'll require a little more work. If you can tackle it in your remaining time, great, if not, I'll take a look when I can.

[tanzoniteblack]

I avoided using go for the macro name because I wanted to make sure it was very clear that this wasn't exactly the same as the core.async macro. Similarly, I chose to use <!-no-throw to make sure that people didn't reach for <! out of habit from core.async , since the error propagation from <!? matches the rest of the deferred ecosystem's usage better.

<!? was chosen over just <! out of the same desire to make it more readily apparent that this is not actually core.async and any prior assumptions from that ecosystem are not guaranteed to be true here.

I'm not tied to the names, or Georgian in general, it's just what I went with to prevent the "oh, this is core.async, have a channel!" confusion.

I didn't create a put!/>! equivalent because I'm not sure what the equivalent would actually be in manifold, nor have we run into a situation yet where we needed an equivalent.

[KingMob]

Yeah, I'd be concerned about confusion, but I'd still prefer English for widespread consumption. "go" isn't inherently a good name anyway...maybe "flow"? It's different, but sounds similar. It matches let-flow, which is already in manifold, but maybe that implies it's too close to let-flow.

<!? is cool, it mimics David Nolen's (and others?) original gists that demonstrated rethrowing an exception after taking in core.async.

Puts would be tricky, because core.async lacks an error model. We'd have to alter it to specify whether it's a success or error for putting to a deferred.

It would also be nice if tsasvla supported streams and alternate thread pools.