-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
[RFC] Fork support #184
[RFC] Fork support #184
Conversation
tests/ExtEventLoopTest.php
Outdated
@@ -57,9 +62,6 @@ public function writeToStream($stream, $content) | |||
fwrite($stream, $content); | |||
} | |||
|
|||
/** | |||
* @group epoll-readable-error | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, this should not be removed. I will revert it with the next commit.
- Use socket pairs for streams (ExtUvLoop cannot handle file streams) - Utilize callable never expectation
Fork support added for all common loop implementations:
I did not implement fork support for the other's, since they only work with outdated PHP versions that nobody should use anymore. |
In my opinion the |
Yep seems legit. The return type of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tux-rampage Thank you for filing this PR and aiming to start a dicussion around this feature! 👍
Let me start with thanking you for this high quality PR, I eyeballed the implementation and don't see any significant technical concerns.
I understand where you're coming from and why this feature may make sense in its current form, but my major concern would be the complexity of this feature. Introducing this would require a significant documentation effort to describe which APIs to use under which circumstances. On top of this, forking is not supported across all platforms anyway.
While I can definitely see some use for this, it's my understanding that the pcntl_fork()
function is quite low level and therefor not used very often in usual PHP applications, let alone long-running, event-driven PHP applications. Many applications seem to use the higher-level ChildProcess component instead (https://github.com/reactphp/child-process). This will fork
internally and then exec
an arbitrary child process and as such avoids many of the issues with inheriting resources from the parent process (such as the event loop).
This makes me wonder if we might come up with a higher-level abstraction for forking instead. I see you did some work in https://github.com/tux-rampage/react-multiprocess-server already, so perhaps we could use this as a starting point? What do you think about this?
@clue Thank you for your response. You brought up some good points. Actually this originated from https://github.com/tux-rampage/react-multiprocess-server - I tried to implement this with fork and found myself to hack around the Loop's encapsulation, which was neither elegant nor stable. I was looking for some approach to allow multi-process handling of requests to utilize multi CPU systems, inspired by Node's Cluster. Now PHP is different and my naive assumtion is to open the socket on the master process and fork handlers that will listen and accept on this connection. But some event loop implementations require to re-initialize the loop. So to do this the Event loop has to either expose its inner loop resource (if any) or provide behavior to do this. Providing the behavior seemed to be the more elegant solution to me so I created this PR. In the meantime, Joe created a parallel extension, which looks promising for this purpose. It will isolate the parallel runtime so a new loop can be created without worring about the existing one. This would make the this PR obsolete, but it will require parallel without any fallback. Thanks for taking time looking at this. |
@tux-rampage There are a few ways to do that, you can for example open the same socket on multiple processes using Also already started working on a package doing |
@tux-rampage Thank you for digging into this and filing this PR! 👍 This is kind of an old issue that hasn't seen any activity in a while and it's unlikely this will get traction any time soon. It's still my understanding this changeset may provide value for more advanced use cases but at the same time opens up a can of worms due to the PHP's limited access to low-level APIs. PHP provides higher-level APIs for process creation as used in our ChildProcess component. These APIs internally use the underlying Given that, I'll assume this has been answered and will close this for now. If you feel this is still relevant, please come back with more details and we can always keep discussing and/or reopen this 👍 Again thank you for your effort nonetheless, keep it up! 👍 |
@clue Thank your for taking time and your detailed follow up. I fully agree with your points and there seems to be better options out there. As for my use case I'm using amp's cluster package which provides this kind of feature through child-processes or parallel and is interoperable with ReactPHP. Maybe this may help people ending up on this PR for similar concerns as well. Thanks for all your work and contribution to the async PHP world. 🙏🏻 |
At the moment react cannot be used with
pcntl_fork()
out of the box at the moment.This mostly applies to scenarios where a fork is performed when the loop is already running and active. For example from an registered SIGCHLD signal istener.
This PR offers the following proposal to make this possible:
Event loops that support forking will implement
ForkableLoopInterface
. Users can theninstanceof
-check if they can use fork safely.After calling
pcntl_fork()
the user has two options in the child process:ForkableLoopInterface::recreateForChildProcess()
. This will detach everything from the loop, stop it and return a new loop instance.Example for recreate: