clj-commons / manifold

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

Add `go-off` macro as alternate way to work with deferreds #200

Closed tanzoniteblack closed 2 years ago

tanzoniteblack commented 3 years ago

Add an alternate to let-flow that allows users to write async code in the same fashion oas core.async/go, but with error propagation. This method completely avoids the various bugs & unexpected issues (like with timeouts) that let-flow's attempt at "magically" dereffing defferables causes.

This macro is used extensively in the backend code base at Yummly and should be considered fairly well battle tested.

tanzoniteblack commented 3 years ago

@KingMob , moving conversation about tsasvla to here.

See these comments for the start of this conversation: https://github.com/clj-commons/manifold/issues/183#issuecomment-882932912 https://github.com/clj-commons/manifold/issues/183#issuecomment-882973183 https://github.com/clj-commons/manifold/issues/183#issuecomment-883525239

tanzoniteblack commented 3 years ago

@KingMob https://github.com/clj-commons/manifold/pull/200/commits/cb755de7234ea95f1317baeb6b744d2be8058410 adds an implementation macro that will allow the user to specify an executor pool, defaulting to ex/execute-pool when the original macro is called

tanzoniteblack commented 3 years ago

@KingMob https://github.com/clj-commons/manifold/pull/200/commits/b61f26eda92a8305c38abe2478f55756f80d091d adds support for reading from streams in a tsasvla block

KingMob commented 3 years ago

Wow, that was fast!

Couple minor notes: "executor" is misspelled, and I think the benchmark test was left commented out.

tanzoniteblack commented 3 years ago

Whoops, good catch on the typo. And you're right, checked in the commenting out of the benchmarks on accident. I'll fix those shortly.

Any updated thoughts on a better name?

tanzoniteblack commented 2 years ago

@KingMob , have you been able to come up with a better name for this macro?

KingMob commented 2 years ago

@tanzoniteblack Well, I'm concerned go will mislead people, because it's not exactly like core.async go. I like flow, but it doesn't exactly work like let-flow does, either. Of the two, go is closer to both the original verb, how it works behind-the-scenes, and how you use it. The key thing with choosing go is to educate people on the differences from core.async.

Unless I hear a better alternative, it'll probably be go, but I'm open to hearing alternatives. (Proceed? Advance? Move? Vamoose? Scram? Wend?)

tanzoniteblack commented 2 years ago

How about glide? A somewhat synonym of flow, but also starts with a g to make it more reminiscent of go.

KingMob commented 2 years ago

Hmm, maybe, maybe.

"Manifold" itself means "many". What about some form of motion involving multiples, like disperse, leave, spread, distribute, or separate?

Should we just call it go-off? That evokes go, indicates it's different, and is technically more accurate, in that we're going off and computing the body elsewhere.

tanzoniteblack commented 2 years ago

I'm good with go-off. If you give the go-ahead I'll update this PR later to use that instead of tsasvla (which I still like, but I'm a bit of a polyglot with a learning language obsession, so I recognize that's probably not a universally appealing option 😆 )

KingMob commented 2 years ago

Let's do go-off. I like that it sounds a bit cheeky, too.

Can you also rename <!-no-throw to <!? I think Manifold should have parity with core.async there, but as noted in the docstring, we'll suggest reaching for <!? first.

tanzoniteblack commented 2 years ago

Let's do go-off. I like that it sounds a bit cheeky, too.

Can you also rename <!-no-throw to <!? I think Manifold should have parity with core.async there, but as noted in the docstring, we'll suggest reaching for <!? first.

renaming <!-no-throw to <! wouldn't really give us parity. <! will silently return nil in cases an exception happened, <!-no-throw will instead return a throwable. We could create an additional macro for <! that will silently swallow the errors if we really care about parity, but I think that might be going out of our way to cause unnecessary difficulties & maintenence.

KingMob commented 2 years ago

@tanzoniteblack

To clarify, I was thinking more about parity of the intended usage, to minimize the discrepancy between expectations and reality, and less about the actual return behavior.

E.g., because <!? rethrows, it mimics the original macro I saw posted for core.async (David's iirc).

And while it's true that <! in core.async won't automatically return a Throwable if one happens elsewhere, that's ok, because the usage intention is "return exactly what I got, no extra behaviors". Then the difference is that manifold, unlike core.async, has an error model and expects Exceptions to propagate around. If you were to use core.async to directly put Exceptions on channels every time you got them, it would be the same idea.

tanzoniteblack commented 2 years ago

@KingMob , I updated this to use go-off instead of tsasvla for the name of the macro/files/in the documentation. Also updated <!-no-throw to be just <!.

KingMob commented 2 years ago

@tanzoniteblack Thanks Ryan, this is coming together really nicely.

I think the only major thing left to consider is conditional compilation. Should we do it? I.e., if they don't add a core.async dep, they couldn't use these fns/macros.

Pros: Minimize core.async version conflicts, and they don't pay for core.async if not using it Cons: Might confuse some people, may interfere with Graal(?)

Thoughts? I'm considering asking people on #aleph if they have a strong opinion one way or the other.

Also, I'm moving to Portugal tomorrow, so I will be AWOL for the rest of the week, but let's try to wrap this up next week.

KingMob commented 2 years ago

@tanzoniteblack As mentioned in Slack, I'm going to go with :scope "provided" and just keep it simple. Any last-minute thoughts? I know this has taken a while, and I appreciate your patience.

tanzoniteblack commented 2 years ago

@KingMob , everything looks good to me 👍