amphp / amp

A non-blocking concurrency framework for PHP applications. 🐘
https://amphp.org/amp
MIT License
4.25k stars 257 forks source link

[Feature Request] - `awaitAll` throwing `CompositeException` #442

Open rela589n opened 3 months ago

rela589n commented 3 months ago

Hello,

First of all, I'd like to thank you for all the effort you spent implementing and supporting such an wonderful library. I'm very excited due to the fact that third version has been released and now every project could benefit from AMP.

Now, using it on one of my last projects, I'd like to ask for a small feature that would slightly improve DX.

Consider this example:

private function createEvent(ResetUserPasswordCommand $command): UserPasswordResetEvent
{
    [$user, $passwordResetRequest] = Future\awaitAnyN(2, [
        async(fn (): User => $this->findUser($command)),
        async(fn (): PasswordResetRequest => $this->findPasswordResetRequest($command)),
    ]);

    return new UserPasswordResetEvent($user, $passwordResetRequest, $this->clock->now());
}

In the code above, methods findUser() and findPasswordResetRequest() have two characteristics:

By using async/await, these methods benefit from making two concurrent database requests and besides that we have the full context of thrown exceptions in the error scenario owing to the fact that awaitAnyN collects them into CompositeException.

In case of my project, the calling code has exception handler adapter for CompositeException to show to the end user the detailed information about what went wrong.

This code works perfectly fine, but from developer POV it might not be so convenient to pass $count every time to the function. In addition, if we add new futures to the list, we have not to forget to update the count, and this is just not convenient.

You might say that awaitAnyN is not designed to await all given futures and that I should've used awaitAll function instead.

I agree that awaitAnyN is not so designed for such use case, but in my case, I would like to leverage CompositeException for exception handling, since it provides a neat way to deal with the bunch of exceptions.

Therefore, requested feature could be stated as "awaitAll function throwing CompositeException".

  1. I'm not sure if it's feasible to modify awaitAll, since it would be a BC break;
  2. Another option could be introducing some flag argument for awaitAll, but it's considered as a bad practice and I agree with that;
  3. Maybe creating another function that does the same as awaitAll but throws $errors rather than returning them would be better option given that we leave awaitAll alone.

In any case, please let me know what you think about this feature request and if I could provide a PR for it.

Best regards, Yevhen