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

ExtEvLoop: Add new ExtEvLoop (PECL source ext-ev) #12

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ matrix:
- php: hhvm

install: ./travis-init.sh

script:
- phpunit --coverage-text
- phpunit --coverage-text
Copy link
Member

Choose a reason for hiding this comment

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

This line should be indented by 2, not 4 spaces.

3 changes: 3 additions & 0 deletions script.gdb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
set env MALLOC_CHECK_=3
run
backtrace full
Copy link
Member

Choose a reason for hiding this comment

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

This file, script.gdb, is a leftover and should be removed.

249 changes: 249 additions & 0 deletions src/ExtEvLoop.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,249 @@
<?php

namespace React\EventLoop;

use SplObjectStorage;
use React\EventLoop\Timer\Timer;
use React\EventLoop\Timer\TimerInterface;
use React\EventLoop\Tick\FutureTickQueue;
use React\EventLoop\Tick\NextTickQueue;

class ExtEvLoop implements LoopInterface
{
private $loop;
private $nextTickQueue;
private $futureTickQueue;
private $timers;
private $readEvents = array();
private $writeEvents = array();

private $running = false;

public function __construct()
{
$this->loop = new \EvLoop();
$this->timers = new SplObjectStorage();
$this->nextTickQueue = new NextTickQueue($this);
$this->futureTickQueue = new FutureTickQueue($this);
$this->timers = new SplObjectStorage();
}

/**
* Add a readable stream to the event loop
* @param stream $stream PHP stream resource
* @param callable $listener Callback
* @return void
*/
public function addReadStream($stream, callable $listener)
Copy link
Member

Choose a reason for hiding this comment

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

You should use @inheritdoc for the phpdoc headers of methods defined in the React\EventLoop\LoopInterface.

{
$this->addStream($stream, $listener, \Ev::READ);
}

/**
* Add a writable stream to the event loop
* @param stream $stream PHP stream resource
* @param callable $listener Callback
* @return void
*/
public function addWriteStream($stream, callable $listener)
{
$this->addStream($stream, $listener, \Ev::WRITE);
}

/**
* Remove a readable stream from the event loop.
* @param stream $stream PHP stream resource
* @return void
*/
public function removeReadStream($stream)
{
$key = (int) $stream;
if(isset($this->readEvents[$key])) {
$this->readEvents[$key]->stop();
unset($this->readEvents[$key]);
}
}

/**
* remove a writable stream from the event loop
* @param stream $stream PHP stream resource
* @return void
*/
public function removeWriteStream($stream)
{
$key = (int) $stream;
if(isset($this->writeEvents[$key])) {
$this->writeEvents[$key]->stop();
unset($this->writeEvents[$key]);
}
}

/**
* remove a stream from the event loop
* @param stream $stream PHP stream
* @return void
*/
public function removeStream($stream)
{
if (isset($this->readEvents[(int)$stream])) {
$this->removeReadStream($stream);
Copy link
Member

Choose a reason for hiding this comment

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

I know using isset() here could help us avoiding at least one useless method dispatch (when the stream is actually registered in the event loop), but I guess we can remove it and just call removeReadStream() and removeWriteStream().

}

if (isset($this->writeEvents[(int)$stream])) {
$this->removeWriteStream($stream);
}
}

/**
* Wraps the listener in a callback which will pass the
* stream to the listener then registers the stream with
* the eventloop.
*
* @param stream $stream PHP Stream resource
* @param callable $listener stream callback
* @param bit $flags flag bitmask
Copy link
Member

Choose a reason for hiding this comment

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

Please use resource instead of stream and integer instead of bit for the argument types.

*/
private function addStream($stream, callable $listener, $flags)
{
$listener = function ($event) use ($stream, $listener) {
call_user_func($listener, $stream);
Copy link
Member

Choose a reason for hiding this comment

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

The callback should be invoked by passing the current loop instance as the 2nd argument, so this line should be call_user_func($listener, $stream, $this);.

};

$event = $this->loop->io($stream, $flags, $listener);

if (($flags & \Ev::READ) === $flags) {
$this->readEvents[(int)$stream] = $event;
} elseif (($flags & \Ev::WRITE) === $flags) {
$this->writeEvents[(int)$stream] = $event;
}
}

/**
* {@inheritdoc}
*/
public function addTimer($interval, callable $callback)
{
$timer = new Timer($this, $interval, $callback, false);
$this->setupTimer($timer);

return $timer;
}

/**
* {@inheritdoc}
*/
public function addPeriodicTimer($interval, callable $callback)
{
$timer = new Timer($this, $interval, $callback, true);
$this->setupTimer($timer);

return $timer;
}

/**
* {@inheritdoc}
*/
public function cancelTimer(TimerInterface $timer)
{
if (isset($this->timers[$timer])) {
/* stop EvTimer */
$this->timers[$timer]->stop();

/* defer timer */
$this->nextTick(function() use ($timer) {
$this->timers->detach($timer);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: why detaching the timer on nextTick()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to avoid segfaults that occur on PHP 5.4, 5.5. There is some delay needed before the call to detach the time is actually made - hacky, but it does solve the issue.

Copy link
Member

Choose a reason for hiding this comment

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

For future reference, I'd put in the comment a short explanation for the reason why we need nextTick() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good point, i'll get a comment put in there for future reference.

});
Copy link
Contributor

Choose a reason for hiding this comment

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

hey, this defer was added to work around a segfault, right? is it still needed?

}
}

/**
* Add timer object as
* @param TimerInterface $timer [description]
* @return [type] [description]
*/
Copy link
Member

Choose a reason for hiding this comment

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

Missing something here? :-) Seems incomplete....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks ok to me, what strikes you as incomplete? There are some work arounds necessary with the EV extension in order to avoid segfaults across different versions of PHP, this is what your seeing here.

Copy link
Member

Choose a reason for hiding this comment

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

Hah sorry, I mean the contents of the phpdoc block :-)
See Add timer object as, [description] and type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, missed that - i'll get that updated accordingly

private function setupTimer(TimerInterface $timer)
{
$callback = function () use ($timer) {
call_user_func($timer->getCallback(), $timer);

if (!$timer->isPeriodic()) {
$timer->cancel();
}
};

$interval = $timer->getInterval();

$libevTimer = $this->loop->timer($interval, $interval, $callback);

$this->timers->attach($timer, $libevTimer);

return $timer;
}

public function isTimerActive(TimerInterface $timer)
{
return $this->timers->contains($timer);
}


/**
* {@inheritdoc}
*/
public function nextTick(callable $listener)
{
$this->nextTickQueue->add($listener);
}

/**
* {@inheritdoc}
*/
public function futureTick(callable $listener)
{
$this->futureTickQueue->add($listener);
}


public function tick()
Copy link
Member

Choose a reason for hiding this comment

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

Missing phpdoc headers with @inheritdoc, here and on top of run(), stop() and isTimerActive().

{
$this->nextTickQueue->tick();
$this->futureTickQueue->tick();

$flags = \Ev::RUN_ONCE;
if (!$this->running || !$this->nextTickQueue->isEmpty() || !$this->futureTickQueue->isEmpty()) {
$flags |= \Ev::RUN_NOWAIT;
} elseif (!$this->readEvents && !$this->writeEvents && !$this->timers->count()) {
$this->running = false;
return;
}
$this->loop->run($flags);
}

public function run()
{
$this->running = true;
while($this->running) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd put an empty line before this while loop.

$this->tick();
}
}

public function stop()
{
$this->running = false;
}

public function __destruct()
{
// mannually stop all watchers
foreach($this->timers as $timer) {
$this->timers[$timer]->stop();
}

foreach($this->readEvents as $event) {
$event->stop();
}

foreach($this->writeEvents as $event) {
$event->stop();
}
}
}
40 changes: 40 additions & 0 deletions tests/ExtEvLoopTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace React\Tests\EventLoop;

use React\EventLoop\ExtEvLoop;

class ExtEvLoopTest extends AbstractLoopTest
{
private $file;

public function createLoop()
{
if (!class_exists('EvLoop')) {
$this->markTestSkipped('ev tests skipped because pecl/ev is not installed.');
}

return new ExtEvLoop();
}

public function tearDown()
{
if (file_exists($this->file)) {
unlink($this->file);
}
}

public function createStream()
{
$this->file = tempnam(sys_get_temp_dir(), 'react-');

$stream = fopen($this->file, 'r+');

return $stream;
}

public function testEvConstructor()
{
$loop = new ExtEvLoop();
}
}
12 changes: 12 additions & 0 deletions travis-init.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ if [ "$TRAVIS_PHP_VERSION" != "hhvm" ]; then
popd
echo "extension=libevent.so" >> "$(php -r 'echo php_ini_loaded_file();')"

# install 'pecl-ev' PHP extension
git clone http://bitbucket.org/osmanov/pecl-ev.git
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why fetching the library from its repository when it's available on PECL, but when I tried installing it via PECL on my computer I received this message:

downloading ev-0.2.12.tgz ...
Starting to download ev-0.2.12.tgz (96,837 bytes)
.....................done: 96,837 bytes
could not extract the package.xml file from "/build/buildd/php5-5.5.9+dfsg/pear-build-download/ev-0.2.12.tgz"
Download of "pecl/ev" succeeded, but it is not a valid package archive
Error: cannot download "pecl/ev"
Download failed
install failed

Is this the reason why you prefer cloning from the repository, or is it just my dev environment acting weird?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an artifact from debugging the segfaults in the earlier versions of the extension with PHP 5.6, needed to test against the latest version. I'll test again against the pecl install to see if I can replicate this issue.

# 0.2.10
pushd pecl-ev
phpize
./configure
make
# make test
make install
popd
echo "extension=ev.so" >> "$(php -r 'echo php_ini_loaded_file();')"

# install 'libev' PHP extension
git clone --recursive https://github.com/m4rw3r/php-libev
pushd php-libev
Expand Down