Amp\disperse() function#460
Conversation
|
What is that??? |
4270738 to
3a32eb7
Compare
3a32eb7 to
85eb948
Compare
|
@xpader , if you don't find this useful at all, you are the contributor, and I'm a layman - it's in your authority to close. |
|
Maybe this's not the best name for such a function. |
Security: - Fix HTTP/2 ping flood on active streams (amphp/http-server#386) Bug fixes: - Use VarString for string params in binary protocol (amphp/mysql#142) - Decode BIT columns as int instead of string (amphp/mysql#138) - Close connections on pool destruct (amphp/http-client#396) - Fix duplicate keys in byte-stream split() (amphp/byte-stream#113) - Fix Closure type annotation for static analysis (amphp/amp#451) - Safely handle DisposedException on unsubscribe (amphp/redis#100) Features: - Add TLS support for Redis connections (amphp/redis#98) - Add disperse() for concurrent closure execution (amphp/amp#460)
|
Hi, @webpatser, do you think it's a good thing to include? |
|
Hi, guys |
|
Hi, @xpader ! |
|
@rela589n I don't thing we shoud add this to amp library, you can use it yourself. |
|
I hope you're doing well. Of course, I could have added this to my repo; I would not have opened the PR then. Yet, I think you wouldn't want to bother yourself with For example, consider a cache warmup scenario with disperse($cacheWarmupFunctions);Without $futures = array_map(async(...), $cacheWarmupFunctions);
awaitAll($futures);Which obscures the high picture. I don't think you'd want to write this every time if you can avoid writing this all the time. |
|
Hey @rela589n, sorry for the slow turnaround, your ping slipped past me. Cheers @xpader for chiming in. My honest take: I don't think Future\await(array_map(async(...), $closures));That already reads cleanly once The cache-warmup example is fair. That's a great fit for a small helper in app code or a userland package, where you can name it whatever fits your team. Not a hard no. If the maintainers want it in, I won't push back. But my vote leans toward closing and keeping the helper in user land. |
|
I think there's value in having such a function, you don't want to write that array_map repeatedly. I'm however not sure on the name:
There likely is a better name. And we should be really sure it's a good name if we add it to the core library, because we can't just release another major version of it if we're wrong. I'm totally happy to have this sit around for a while, if someone comes good with naming suggestions and it's still good after a while, we can proceed and merge this. |
|
What do you think about "scatter"? It means: (of a group of people or animals) separate and move off quickly in different directions. |
|
I think it needs to somehow cover the dispersion and then collecting the results again, thought about |
|
If we consider forkJoin, at first, I didn't really think of "fork" as of a verb, but of a noun.
TBH, it reminds me of Scatter-Gather (Not that I ever used it, but it clicked in me) enterprise pattern:
In our case, it is: $results = \Amp\scatterGather([
fn () => $httpClient->request(new Request('https://www.google.com', 'HEAD')),
fn () => $httpClient->request(new Request('https://www.bing.com', 'HEAD')),
]); |
|
Hey @rela589n, @kelunik, on naming.
My vote is |
|
IMO $results = \Amp\concurrent([
fn () => $httpClient->request(new Request('https://www.google.com', 'HEAD')),
fn () => $httpClient->request(new Request('https://www.bing.com', 'HEAD')),
]); |
|
Another option I think of is $results = \Amp\reduce([
fn () => $httpClient->request(new Request('https://www.google.com', 'HEAD')),
fn () => $httpClient->request(new Request('https://www.bing.com', 'HEAD')),
]);Or $results = \Amp\apply([
fn () => $httpClient->request(new Request('https://www.google.com', 'HEAD')),
fn () => $httpClient->request(new Request('https://www.bing.com', 'HEAD')),
]);Or, talking about evaluation, I would even think about $results = \Amp\collapse([
fn () => $httpClient->request(new Request('https://www.google.com', 'HEAD')),
fn () => $httpClient->request(new Request('https://www.bing.com', 'HEAD')),
]);In such a case, the concurrency is the internal detail. We know that Amp is all about concurrency in the first place, and |
|
Why are we only talk about name? I still don't get why just not use |
|
@xpader The use case is when you start from closures rather than Futures. Cache warmup, fan-out HTTP heads, batch job dispatch. Today that's:
The helper collapses the For the record I'm not sold it earns a core slot either, said as much earlier in the thread. But the "why not just |
|
@webpatser I always thought there were more complicated scenarios that I couldn’t understand. So it's just turn closures into Futures and await them? Just because don't want write that code like |
|
@xpader , do you want me to close it up and shut down? |
In this pull request, I want to introduce a
disperse($closures)function.It is created to asynchronously execute an array of closures when the client needs all results.
Initially, I considered placing this function in
Amp\Futurenamespace, but later I opted in favor ofAmp, since this function doesn't have Futures in the signature.