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

Add deferred/alt #102

Merged
merged 1 commit into from Jan 17, 2017
Merged

Add deferred/alt #102

merged 1 commit into from Jan 17, 2017

Conversation

dm3
Copy link
Contributor

@dm3 dm3 commented Sep 13, 2016

This adds a deferred/alt combinator. Naming mirrors the core.async/alt.. functions. Also alt has the same length as zip, its dual. I find it aesthetically pleasing :)

I find it occasionally useful. Would you consider pulling into Manifold?

@danielcompton
Copy link
Member

danielcompton commented Sep 13, 2016

core.async/alt randomises the order that alt's are chosen in if several are available, unless :default is provided also. Randomising the order would probably be good here too?

@dm3
Copy link
Contributor Author

dm3 commented Sep 14, 2016

@danielcompton could you please explain why randomization is useful in this case? I can't find any rationalization for why Core.Async does that.

@danielcompton
Copy link
Member

Not sure if I can put it into words well, but the intuition is to avoid accidentally ordering things which shouldn't be ordered, or depending on the order that ports are declared in an alt.

Say you were testing with dummy data and you filled all your channels up, then ran the alt. If it took the first every time then you may miss subtle concurrency bugs that would occur in production.

@dm3
Copy link
Contributor Author

dm3 commented Sep 14, 2016

This makes sense. I was thinking along the same lines. I see two options then:

  • rename the function to deferred/any and match the CompletableFuture#anyOf which doesn't randomize but also doesn't specify anything in the docs.
  • keep it as deferred/alt and randomize to match the Core.Async intuition

I have no strong feelings either way.

src/manifold/deferred.clj Show resolved Hide resolved
[& vals]
(let [d (deferred)]
(clojure.core/loop [s vals]
(when-let [x (first s)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this pick a random ordering?

@ztellman
Copy link
Collaborator

ztellman commented Oct 3, 2016

Hi, I'm sorry for not responding to this sooner. I think I'd prefer the randomized approach, and alt as the name.

@dm3
Copy link
Contributor Author

dm3 commented Oct 4, 2016

Randomized now.

I used the random-array-filling technique from core.async (except the storing Random in a thread local part). Not sure if that requires a Copyright notice in the sources and whether you'd like to go with that or use another method.

@ztellman ztellman merged commit 908458b into clj-commons:master Jan 17, 2017
@ztellman
Copy link
Collaborator

Sorry for the delay in merging this, I'll be cutting a release soon.

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

Successfully merging this pull request may close these issues.

None yet

3 participants