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 1 commit
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
Prev Previous commit
Next Next commit
updates based on code review
  • Loading branch information
steverhoades committed Nov 4, 2014
commit 34b51b68606f0045a22a4462c350b99fbd661c31
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@ matrix:
install: ./travis-init.sh

script:
- phpunit --coverage-text
- phpunit --coverage-text
3 changes: 0 additions & 3 deletions script.gdb

This file was deleted.

75 changes: 35 additions & 40 deletions src/ExtEvLoop.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class ExtEvLoop implements LoopInterface
{
private $loop;
private $nextTickQueue;
private $futureTickQueue;
private $futureTickQueue;
private $timers;
private $readEvents = array();
private $writeEvents = array();
Expand All @@ -29,84 +29,67 @@ public function __construct()
}

/**
* Add a readable stream to the event loop
* @param stream $stream PHP stream resource
* @param callable $listener Callback
* @return void
* {@inheritdoc}
*/
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
* {@inheritdoc}
*/
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
* {@inheritdoc}
*/
public function removeReadStream($stream)
{
$key = (int) $stream;
if(isset($this->readEvents[$key])) {
$this->readEvents[$key]->stop();
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
* {@inheritdoc}
*/
public function removeWriteStream($stream)
{
$key = (int) $stream;
if(isset($this->writeEvents[$key])) {
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
* {@inheritdoc}
*/
public function removeStream($stream)
{
if (isset($this->readEvents[(int)$stream])) {
$this->removeReadStream($stream);
}

if (isset($this->writeEvents[(int)$stream])) {
$this->removeWriteStream($stream);
}
$this->removeReadStream($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
*
* @param resource $stream PHP Stream resource
* @param callable $listener stream callback
* @param int $flags flag bitmask
*/
private function addStream($stream, callable $listener, $flags)
{
$listener = function ($event) use ($stream, $listener) {
call_user_func($listener, $stream);
call_user_func($listener, $stream, $this);
};

$event = $this->loop->io($stream, $flags, $listener);
Expand Down Expand Up @@ -151,13 +134,13 @@ public function cancelTimer(TimerInterface $timer)

/* defer timer */
$this->nextTick(function() use ($timer) {
$this->timers->detach($timer);
$this->timers->detach($timer);
});
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
* 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

Expand All @@ -180,6 +163,9 @@ private function setupTimer(TimerInterface $timer)
return $timer;
}

/**
* {@inheritdoc}
*/
public function isTimerActive(TimerInterface $timer)
{
return $this->timers->contains($timer);
Expand All @@ -202,7 +188,9 @@ public function futureTick(callable $listener)
$this->futureTickQueue->add($listener);
}


/**
* {@inheritdoc}
*/
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();
Expand All @@ -218,14 +206,21 @@ public function tick()
$this->loop->run($flags);
}

/**
* {@inheritdoc}
*/
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();
}
}

/**
* {@inheritdoc}
*/
public function stop()
{
$this->running = false;
Expand All @@ -234,15 +229,15 @@ public function stop()
public function __destruct()
{
// mannually stop all watchers
foreach($this->timers as $timer) {
$this->timers[$timer]->stop();
foreach ($this->timers as $timer) {
$this->timers[$timer]->stop();
}

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

foreach($this->writeEvents as $event) {
foreach ($this->writeEvents as $event) {
$event->stop();
}
}
Expand Down