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 'firstCompleted' and 'firstCompetedFuture' to 'asyncfutures2' #339

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zah
Copy link
Contributor

@zah zah commented Dec 16, 2022

These are useful alternatives to the race helper that make it easy to implement "first successful response" strategies.

Currently, the Nimbus VC is trying to implement such a strategy, but it has the issue that if some of the servers responds too quickly with an error, the successful responses that might complete later will be ignored.

@arnetheduck
Copy link
Member

this looks pretty specialistic - ie the typical pattern for race is to do so in a loop until the desired result is achieved among the futures that were passed to it, removing the ones that were dissatisfactory - it feels like we keep adding special cases here instead of having a more limited amount of fundamental primitives that compose

@arnetheduck
Copy link
Member

iff we're going to add this feature, it should indeed be implemented as such, ie a while loop over a race-like primitive so we don't have to have this wall of logic, and to highlight that this is syntactic sugar for a special case

@zah
Copy link
Contributor Author

zah commented Dec 16, 2022

A loop over race will be significantly more expensive (it will feature unnecessary cancellations, O(n^2) in memory copying operations, etc). It's not clear what is gained exactly in return. The code size argument seems weak - it's not even clear whether the loop woudn't be similar in size and complexity.

@arnetheduck
Copy link
Member

well, nonetheless this adds to a jungle of special functions, none of which quite seems to do the right thing - I'd consider it a UX degradation of the futures library to add this for this reason, ie the gain is mostly insignificant - o(n2) doesn't matter, we're not talking about tens of thousands of futures (chronos is not able to do that effeiciently for a number of other reasons).

What is gained is that you can begin to understand the difference between the various pre-composed special cases, ie what they actually do - the differences between them are so subtle and nuanced that it's hard to pick one.

@arnetheduck
Copy link
Member

This pr would be a lot more palatable if we didn't have race and 20 other future list combinators already - the fact that we have so many speaks of this needing a review with removals rather than more and more additions

@zah
Copy link
Contributor Author

zah commented Dec 16, 2022

I would further argue that this helper is present in most async run-times. Here is the JavaScript version for example:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/any

@arnetheduck
Copy link
Member

Then perhaps we should deprecate race?

@zah
Copy link
Contributor Author

zah commented Dec 16, 2022

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.

2 participants