status-im / nim-chronos

Chronos - An efficient library for asynchronous programming
https://status-im.github.io/nim-chronos/docs/chronos
Apache License 2.0
353 stars 51 forks source link

Add 'firstCompleted' and 'firstCompetedFuture' to 'asyncfutures2' #339

Open zah opened 1 year ago

zah commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Then perhaps we should deprecate race?

zah commented 1 year ago

Here is race in JavaScript:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/race