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

TProcessHelper and TSignalsDispatcher #972

Closed
belisoful opened this issue Jun 6, 2023 · 14 comments
Closed

TProcessHelper and TSignalsDispatcher #972

belisoful opened this issue Jun 6, 2023 · 14 comments

Comments

@belisoful
Copy link
Member

There is a problem with pcntl_fork a PRADO application: PDO connections are replicated and closing one closes all of them. In PDO, it is a feature; when the PDO object is dereferenced (aka, an object is done and no longer being used) it closes the connection. But after forking, there are two instances of the PDO representing a single connection. There is no way to exit a child thread without ending the parent PDO connection because the child PDO object is dereferenced.

The general solution is to close all connection in the parent, then reopen after the fork, as necessary, in the parent and child.

Proposal: Prado\Util\Helpers\TProcessHelper::fork that raises the global event fxPrepareForFork, then calls pcntl_fork, then raises the global event fxRestoreAfterFork.

This requires updating TDbConnection to be AutoGlobalListen = true. fxPrepareForFork will set Active = false for all db connections. It will also return [TDbConnection::objectId => bool "active status"]. The object ID is spl_object_id.

What are your thoughts on adding AutoglobalListen overhead to TDbConnections for this feature that will only be used in a rare instance?

@belisoful
Copy link
Member Author

TDbConnection could attach and detach event handlers when Active = true and removed when Active = false. (active = false for the fxPrepareForFork does not remove the global event handlers of TDbConnection, of course)

@belisoful
Copy link
Member Author

There is no overhead of auto global listen if the events handlers are added when active = true and removed when active = false on the TDbConnection.

Are there any other objects that need to be "corrected" for forking?

@ctrlaltca
Copy link
Member

A quick solution would be to subclass TDbConnection with a single method enabling AutoglobalListen.
A more comprehensive solution would be to make it somewhat configurable in application.xml, so that each class, when instanced, can detect if it needs to enable AutoGlobalListen. This could probably lead to some performance gains in applications that doesn't use fx events at all.

@ctrlaltca
Copy link
Member

Are there any other objects that need to be "corrected" for forking?

Anything using php resources i guess, eg. fopen().

@belisoful
Copy link
Member Author

That's a great suggestion. the new IO package encapsulates the fopen, socket_create, popen, and proc_open. They do not exactly have the same issues with auto-destructing on dereference. They can be dereferenced without closing.

However, tmpfile() does actually do the same thing. there is a TTempStream class for encapsulating that function. It is a derivative of fopen. tmpfile does actually do the auto remove on dereference.

hmmm. Thank you for your comment on this. It makes me think that we should use a PRADO managed tmp file as default rather than tmpfile() as a default due to tmpfile not being thread safe, as per your indicator.

@belisoful
Copy link
Member Author

Let me check that fopen Dereferenced resource does orphan the resource in the child...

@belisoful
Copy link
Member Author

belisoful commented Jun 7, 2023

ChatGPT says (and verifies itself) that a Child process exiting without fclose a file/stream resource will preserve the resource in the parent. The the tmpfile() will close its resource automatically on child exit because it works like PDO to auto self destruct.

@belisoful
Copy link
Member Author

I like the idea of a behavior adding these event handlers (of the owner). This would be the performant way. TDataSourceConfig module having module behaviors (passed to the TDbConnection) that would attach the event handlers. I likeTForkableBehavior as a name for adding these event handlers to the suggested global events.

And the TTempStream, when using tmpfile(), the class needs to copy the tmpfile() on fxPrepareForFork and pass it to fxRestoreAfterFork. The parent then copies its own tmpfile to a new tmpfile, and the child copies its tmpfile to its new tmpfile(). When the original tmpfile and original tmpfile copy (for the child) are dereferenced, they are unlink and then both the parent and child maintain their new tmpfile.

The automatic file deletion can be turned off for Prado managed tmp files and not need to do all that copying of tmp files.

@belisoful
Copy link
Member Author

belisoful commented Jun 7, 2023

I found this interesting code and comment at https://www.php.net/manual/en/function.pcntl-fork.php#127790

// Explicity kill process, or else the resource shared with parent will be closed
posix_kill(getmypid(), SIGTERM);
exit();

This may be the solution to PDO and tmpfile auto-detonating on dereferencing in children.

This may render the global events for this unnecessary.... testing.

@belisoful
Copy link
Member Author

belisoful commented Jun 7, 2023

The way I'm thinking of implementing this, if it works, as an extension of the TExitException.

@belisoful
Copy link
Member Author

If the TChildExitException with a Terminate Signal is used, the app resources shouldn't be replicated. and visa versa. I'm thinking that an app level behavior is needed. The TForkable[Behavior] on an application would add the TForkable to TTDbConnection class behaviors, which adds its fxPrepareForFork and fxRestoreAfterFork events to the global event handler.

This is getting interesting with behaviors.

@belisoful
Copy link
Member Author

belisoful commented Jun 9, 2023

Here is my latest thinking:

  1. if TDbConnection goes Active = false, on fxPrepareForFork and Active = true on fxRestoreAfterFork, then calling SIG_TERM on exit for the child will leave the newly open child DB hanging. so a third global event for closing up child processes to clean up any resources that need to be closed before SIG_TERM is sent and process exit.
  2. if TDBConnection is not closed and reopened on forking, then SIG_TERM is acceptable.

Here is the implementation::

  • Closing a forked child process should call TChildExitException, which gracefully closes the child [app] fork and raises fxExitChildProcess before exit(); in TApplication::run
  • our function for pcntl_fork should raise fxPrepareForFork before and fxRestoreAfterFork after forking. The pid should be included in the parameter. As of now, fxPrepareForFork return values are [spl_object_id => object data] which is then merged by replace (to retain numeric keys), The merged array of object is set with the 'processid' and sent to
  • TForkable behavior attaches the event handlers of the owner to the global events.
  • TDataSourceConfig needs to pass through its behaviors to the TDbConnection. Or set up in TBehaviorsModule on TDbConnection.
  • RFI: TModule::init loads [configuration] behaviors. #971 should go along with this issue because the behaviors of TDataSourceConfig (passing through to TDbConnection) use the changes from RFI: TModule::init loads [configuration] behaviors. #971 to capture module behaviors

@belisoful
Copy link
Member Author

After lots of research, the new PHP8 doesn't have resource issues in forking. I tried making a child corrupt the parent PDO Sqlite3 connection and a tmpfile resource but failed to recreate the issues I was having in prior versions of PHP. It was easy to fork a cron thread that exited and corrupted the parent PDO DB Connection. Not so much now.

Maybe PHP is incrementing resource reference counters so they do not deference on exiting the child. Whatever it is, these solutions solve no problem.

Closing issue.

@belisoful
Copy link
Member Author

belisoful commented Jun 18, 2023

After much thinking, I think there is a use case for these methods... Capturing the Application log of children and injecting it back into the main thread.

An application behavior that connects the two threads would transfer the log from the child to the parent at the end of execution.

@belisoful belisoful reopened this Jun 18, 2023
@belisoful belisoful changed the title RFC: Global Event fxPrepareForFork and fxRestoreAfterFork TProcessHelper::fork Jul 21, 2023
@belisoful belisoful changed the title TProcessHelper::fork TProcessHelper and TSignalsDispatcher Jul 21, 2023
belisoful added a commit to belisoful/prado that referenced this issue Jul 24, 2023
… & functions

- TProcessHelper for process related functions like forking (pcntl_fork) and isSystemWindows().
- TSignalsDispatcher for raising signal related global events from signals for multiple handlers per signal, alarm and disarm callbacks at a specific time (1 second precision), and for callbacks per child processes (proc_open, pcntl_fork, etc) when they end (or "stop and start"?).
- ISingleton is a new interface for application singleton objects
- TShellWriter is updated to use the TProcessHelper::isSystemWindows function
-TComponent only hasMethod of global event handlers that do actually exist.  As a global event it exists always, but not the method.
- TEventSubscription example is updated to reflect TSignalsDispatcher and TSignalParameter
- TApplicationSignals behavior for configuration and attaching of TSignalsDispatcher
- TCaptureForkLog behavior for capturing the logs of forked child processes in the main log.  Effectively a ForkLogRouter but registering as a behavior on TApplication.
- TForkable behavior for attaching the owner's ::fxPrepareForFork() and ::fxRestoreAfterFork() handlers to their respective global events.
-TGlobalClassAware behavior for attaching the owner's ::fxAttachClassBehavior() and ::fxDetachClassBehavior() to their respective global events so they change with global class changes without listening.
- TProcessWindowsPriority specifies the numeric priorities that windows uses for each respective priority level.
- TProcessWindowsPriorityName specifies the text priorities that windows uses for each respective priority level.
ctrlaltca pushed a commit that referenced this issue Aug 1, 2023
…ons (#986)

* #972 TProcessHelper, TSignalsDispatcher, and related classes & functions

- TProcessHelper for process related functions like forking (pcntl_fork) and isSystemWindows().
- TSignalsDispatcher for raising signal related global events from signals for multiple handlers per signal, alarm and disarm callbacks at a specific time (1 second precision), and for callbacks per child processes (proc_open, pcntl_fork, etc) when they end (or "stop and start"?).
- ISingleton is a new interface for application singleton objects
- TShellWriter is updated to use the TProcessHelper::isSystemWindows function
-TComponent only hasMethod of global event handlers that do actually exist.  As a global event it exists always, but not the method.
- TEventSubscription example is updated to reflect TSignalsDispatcher and TSignalParameter
- TApplicationSignals behavior for configuration and attaching of TSignalsDispatcher
- TCaptureForkLog behavior for capturing the logs of forked child processes in the main log.  Effectively a ForkLogRouter but registering as a behavior on TApplication.
- TForkable behavior for attaching the owner's ::fxPrepareForFork() and ::fxRestoreAfterFork() handlers to their respective global events.
-TGlobalClassAware behavior for attaching the owner's ::fxAttachClassBehavior() and ::fxDetachClassBehavior() to their respective global events so they change with global class changes without listening.
- TProcessWindowsPriority specifies the numeric priorities that windows uses for each respective priority level.
- TProcessWindowsPriorityName specifies the text priorities that windows uses for each respective priority level.

* First iteration correcting errors

SignalsDispatcher is adding PID and STATUS to the $sigInfo because it is not passed by the system when invoking the SIGCHLD on the system test instances.  Loosening the test requirement for this test.

Not 100% sure what the process issue is yet.  This is still being investigated

* Checking the test environment for `renice`

This is designed to see what the results of renice on the testing machine and configuration.

* more process priority testing on the github test machine-environment

Adding documentation for the result of renice on various systems.

* Checking renice string result for "denied" to define an error

* Fixed a minor differences in how priorities are handled between MacOS and Linux

Mac OS uses a relative difference of priority while linux seems to use an absolute priority.

* What is the system that the verification happens on?

Basically, what system these tests are run on will help determine if the system will allow a process priority of 20.   If the lowest priority is only 19 on the system then this would produce an error when setting to 20... and it not being 20 but 19.

* corrected the idle priority from 20 to 19 for linux

MacOS apparently does support a priority of 20 but linux does not.

Added isSystemLinux (and isSystemMac in a prior commit too)

* fixed class typos

slightly better Mac handling of priority

* History.md typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants