Conversation
There was a problem hiding this comment.
Please check the following comments.
Remaining concern is that a lambda function in $decide_actoin does not have an interface such as
- Arguments are ambiguous.
- It can return any type of value.
Originally this library is from Perl, so I can accept these ambiguities, but it would be great if you resolve them.
lib/Prefork.php
Outdated
| /** | ||
| * @var callable lamda fuction that decide action | ||
| */ | ||
| public $decide_action = null; |
There was a problem hiding this comment.
You can set string, int or whatever to $decide_action in current implementation, which cause a fatal error here.
This property should be private or protected. And then define mutator for it and validate the argument with is_callable or set an empty lambda function at __construct().
lib/Prefork.php
Outdated
| $action = $currect_workers < $this->max_workers; | ||
| if ( isset($this->decide_action) ) { | ||
| try { | ||
| $action = call_user_func_array($this->decide_action,array($currect_workers, $this->max_workers)); |
There was a problem hiding this comment.
It's not sure $this->decide_action is callable, which cause a fatal error.
lib/Prefork.php
Outdated
| printf("Exception. Use default action: %s\n",$e->getMessage()); | ||
| } | ||
| } | ||
| if ($action) { |
There was a problem hiding this comment.
Do you expect $action is bool to determine the parent process should spawn child processes or not? If so the name $action is ambiguous. It should be $can_spawn, $should_fork or something like that.
lib/Prefork.php
Outdated
| if (count(array_keys($this->worker_pids)) < $this->max_workers) { | ||
| $currect_workers = count(array_keys($this->worker_pids)); | ||
| $action = $currect_workers < $this->max_workers; | ||
| if ( isset($this->decide_action) ) { |
| $cb->($exit_pid, $status); | ||
| } | ||
| */ | ||
| call_user_func_array($cb, array($exit_pid, $status)); |
There was a problem hiding this comment.
Validate $cb is callable if enable this method. $cb is null by default.
lib/Prefork.php
Outdated
| */ | ||
| public $on_child_reap = null; | ||
| /** | ||
| * @var callable lamda fuction that decide action |
There was a problem hiding this comment.
lamda should be lambda.
If action means a function to determine the parent process should spawn child processes or not, this comment should be so.
| ? $args['decide_action'] : null; | ||
| if ( isset($this->decide_action) && !is_callable($this->decide_action) ){ | ||
| die("decide_action should be callable\n"); | ||
| } |
There was a problem hiding this comment.
What I wanted to say is:
- Define setter for
$decide_action. e.g.setDecideAction($function) - Validate
$functionaboveis_callable()in the setter. - No need to accept
$args['decide_action']in__construct()like above.- Or like this.
- Set empty lambda function to
$decide_actionin__construct()to ensure$decide_actionis callable.
Simple usage is like this.
$pp = new Prefork([
...,
]);
$pp->setDecideAction(function (...) use(...) { ... });| $pid = null; | ||
| if (count(array_keys($this->worker_pids)) < $this->max_workers) { | ||
| $currect_workers = count(array_keys($this->worker_pids)); | ||
| $should_fork = $currect_workers < $this->max_workers; |
There was a problem hiding this comment.
You might be able to set default $decide_action in __construct().
// Set default $decide_action in __construct().
$this->decide_action = function($current_workers, $max_workers)) { return $current_workers < $max_workers; }2 participants
decide_action callback can control that parent process create or not create new processes.
With this PR, max-worker can be changed on demand.