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 support for amp v3 #1084

Merged
merged 3 commits into from
May 18, 2023
Merged

Add support for amp v3 #1084

merged 3 commits into from
May 18, 2023

Conversation

veewee
Copy link
Contributor

@veewee veewee commented Apr 28, 2023

Q A
Branch v2
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Documented? no
Fixed tickets #1064

This PR introduces AMP v3 support.
Since it's quite breaking in both (internal) API as supported PHP versions, this will be released as GrumPHP V2.

An extra pair of eyes is welcome!

@veewee veewee marked this pull request as draft April 28, 2023 09:58
@veewee veewee added this to the 2.0.0 milestone Apr 28, 2023
@veewee veewee self-assigned this Apr 28, 2023
@veewee veewee force-pushed the ampv3 branch 6 times, most recently from 53c35c1 to 2b8dfdc Compare April 28, 2023 12:19
@veewee veewee mentioned this pull request Apr 28, 2023
@veewee veewee marked this pull request as ready for review April 28, 2023 14:56
return new TaskResultCollection(
await($futures, $stopOnFailure->cancellation())
);
} catch (CancelledException $e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any other amp related exceptions I should be worried about?
There might be a shortcut to collect all finished results somewhere in amp instead of doing my own reduce?

*/
public function handle(
TaskInterface $task,
TaskRunnerContext $runnerContext,
StopOnFailure $stopOnFailure,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently I have to pass down this Cancellation all over the handlers.
I don't mind, but maybe there is another way?

}

$currentEnv = $_ENV;
$worker = $this->poolFactory->createShared();
$execution = $worker->submit(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error handling of the new worker system seems a lot better.
Do I need to take care of some other amp related exceptions?

(see old implementation -> There I needed to wrap a lot of error handling myself)

*
* @implements Task<TResult, TReceive, TSend>
*/
class SerializedClosureTask implements Task
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This task is rather straight forward, you might want to reconsider supporting it in parallel-functions for v3 anyways. For now, it's here to stay.

@veewee veewee changed the base branch from master to v2.x May 18, 2023 18:10
@veewee
Copy link
Contributor Author

veewee commented May 18, 2023

Merging this one for now.
Feel free to test it out. All feedback is welcome, including answers to the questions above!

@veewee veewee merged commit 29d1dec into phpro:v2.x May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant