Skip to content

Commit

Permalink
Fix segmentation fault on PHP 7+ (#175, #169)
Browse files Browse the repository at this point in the history
* addFunction needs $context param to be a reference, not value. Someone (I have not checked who) ignored docs and passed the second one,
* In PHP 7+ garbage collection would cause a segfault now GearmanExecute::$workersBucket survives GC and prevents segfault.
* Fix the command usage with PHP 7.1.1
* This is a port of #172
* Fix test to not pass context on its own
* Update travis to run PHP 7.*
* Do not allow newest PHPUnit
  • Loading branch information
malarzm authored and daum committed Mar 23, 2017
1 parent a0a39dd commit a69e470
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 18 deletions.
10 changes: 7 additions & 3 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ language: php
php:
- 5.5
- 5.6
- 7.0
- 7.1
- hhvm

matrix:
Expand All @@ -14,9 +16,11 @@ matrix:
install:
- sh -c "sudo apt-get -y install uuid-dev libgearman-dev gearman gearman-job-server"
before_script:
- curl -L -o gearman-0.8.3.tgz http://pecl.php.net/get/gearman/0.8.3
- tar -xzf gearman-0.8.3.tgz
- sh -c "cd gearman-0.8.3 && phpize && ./configure && make && sudo make install && cd .."
- if [[ $TRAVIS_PHP_VERSION = 5.* ]]; then curl -L -o gearman.tgz http://pecl.php.net/get/gearman/1.1.2; fi;
- if [[ $TRAVIS_PHP_VERSION = 7.* ]]; then curl -L -o gearman.tgz https://github.com/wcgallego/pecl-gearman/archive/gearman-2.0.1.tar.gz; fi;
- tar -xzf gearman.tgz
- if [[ $TRAVIS_PHP_VERSION = 5.* ]]; then sh -c "cd gearman-1.1.2 && phpize && ./configure && make && sudo make install && cd .."; fi;
- if [[ $TRAVIS_PHP_VERSION = 7.* ]]; then sh -c "cd pecl-gearman-gearman-2.0.1 && phpize && ./configure && make && sudo make install && cd .."; fi;
- echo "extension=gearman.so" >> `php --ini | grep "Loaded Configuration" | sed -e "s|.*:\s*||"`
- composer install --prefer-source --no-interaction

Expand Down
30 changes: 23 additions & 7 deletions Service/GearmanExecute.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ class GearmanExecute extends AbstractGearmanService
*/
protected $stopWorkSignalReceived;

/**
* Bucket with worker objects configuration for PECL
* @var array
*/
protected $workersBucket = [];

/**
* Construct method
Expand Down Expand Up @@ -303,14 +308,19 @@ private function runJob(\GearmanWorker $gearmanWorker, $objInstance, array $jobs
*/
foreach ($jobs as $job) {

/**
* worker needs to have it's context into separated memory space;
* if it's passed as a value, then garbage collector remove the target
* what causes a segfault
*/
$this->workersBucket[$job['realCallableName']] = [
'job_object_instance' => $objInstance,
'job_method' => $job['methodName'],
'jobs' => $jobs,
];
$gearmanWorker->addFunction(
$job['realCallableName'],
array($this, 'handleJob'),
array(
'job_object_instance' => $objInstance,
'job_method' => $job['methodName'],
'jobs' => $jobs
)
array($this, 'handleJob')
);
}

Expand Down Expand Up @@ -407,8 +417,14 @@ public function executeWorker($workerName, array $options = array())
*
* @return mixed
*/
public function handleJob(\GearmanJob $job, $context)
public function handleJob(\GearmanJob $job)
{
if (!isset($this->workersBucket[$job->functionName()])) {
$context = false;
} else {
$context = $this->workersBucket[$job->functionName()];
}

if (
!is_array($context)
|| !array_key_exists('job_object_instance', $context)
Expand Down
15 changes: 8 additions & 7 deletions Tests/Service/GearmanExecuteTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public function testDispatchingEventsOnJob()
'jobs' => array(
0 => array(
'callableName' => "test",
'methodName' => "test",
'methodName' => "myMethod",
'realCallableName' => "test",
'jobPrefix' => NULL,
'realCallableNameNoPrefix' => "test",
Expand Down Expand Up @@ -103,12 +103,13 @@ public function testDispatchingEventsOnJob()

// Finalize worker mock by making it call our job object
// This is normally handled by Gearman, but for test purpose we must simulate it
$worker->method('work')->will($this->returnCallback(function() use ($service, $object){
$service->handleJob(new \GearmanJob(), array(
'job_object_instance' => $object,
'job_method' => 'myMethod',
'jobs' => array()
));
$job = $this->getMockBuilder('GearmanJob')
->disableOriginalConstructor()
->getMock();
$job->method('functionName')->willReturn('test');

$worker->method('work')->will($this->returnCallback(function() use ($service, $object, $job){
$service->handleJob($job);
return true;
}));

Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@
}
},
"require-dev": {
"phpunit/phpunit": ">=4.8"
"phpunit/phpunit": ">=4.8 <6.0"
}
}

0 comments on commit a69e470

Please sign in to comment.