Skip to content

Conversation

@kpicaza
Copy link
Member

@kpicaza kpicaza commented Jan 23, 2021

Description

Seeing the benchmark in https://github.com/the-benchmarker/web-frameworks we need to allow the option to fork the react-php server in the different threads available in the host machine.

Thanks to @mmoreram for helping me with this issue, and for your great work for the PHP community;- D

@kpicaza kpicaza added the enhancement New feature or request label Jan 23, 2021
@kpicaza kpicaza added this to the 2.0.0 milestone Jan 23, 2021
@kpicaza kpicaza requested a review from a team January 23, 2021 12:55
@kpicaza kpicaza self-assigned this Jan 23, 2021
{
public static function fork(int $numberOfWorkers, callable $asyncServer, int $numberOfFork = 0): void
{
$pid = pcntl_fork();
Copy link
Member

Choose a reason for hiding this comment

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

Just notice that the pcntl_fork function is only available on Unix-based system.

The reference is available here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am searching for an alternative for windows without luck, this feature will only be available on nix systems I think. We can protect it from usage in Windows systems;-S

Copy link
Member Author

Choose a reason for hiding this comment

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

something like this:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm working o mutation tests now;- D

Copy link
Member

Choose a reason for hiding this comment

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

BTW, do you consider using the pthread?

Copy link
Member Author

@kpicaza kpicaza Jan 23, 2021

Choose a reason for hiding this comment

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

@peter279k @mmoreram I've tried parallel extension with @xserrat a time ago, and we discard it for complexity issues(we make it run with a docker image created by @WyriHaximus) antidot-framework/antidot-react-psr15#1, both pthreads and parallel requires PHP compiled with ZTS and extra configuration(enable extension).
I propose to maintain this feature without the windows multi-thread support and opening a new issue to research and then add multi-thread support in windows systems.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Check this. The Parallel extension looks good :).

Choose a reason for hiding this comment

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

@kpicaza In the short term that's the easiest way to make this happen. Really interested to see how well this works with different event loops as this isn't something we've tried in the past.

Choose a reason for hiding this comment

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

@peter279k Yeah, I've build https://github.com/reactphp-parallel around it for a reason 😉 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @WyriHaximus, @mmoreram, and @peter279k for guidance;- D. I've just opened a new issue #14 to add support and document the usage by parallel ext.

@kpicaza kpicaza marked this pull request as ready for review January 23, 2021 18:40
@kpicaza kpicaza requested a review from peter279k January 24, 2021 11:11
@kpicaza kpicaza merged commit b6fb675 into 2.x.x Jan 24, 2021
@kpicaza kpicaza deleted the allow_fork_server branch January 24, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants