Implement ExtEvLoop (PECL ext-ev) by ivkalita · Pull Request #148 · reactphp/event-loop

ivkalita · 2018-01-15T20:38:28Z

Finally I fixed my old pull request. I'm really sorry that it took so long for me.

I hope, that this feature still wanted and needed :)

WyriHaximus · 2018-01-15T20:42:27Z

Hey @kaduev13 yes your PR is still wanted, in fact the plan was to take over and finish your PR once v0.5.0 gone out. Will look into this one later today or tomorrow 👍 .

clue

@kaduev13 Thank you so much for keeping up with this and filing an update! 👍

I've found some minor issues below, otherwise LGTM 👍

clue · 2018-01-16T15:09:20Z

src/ExtEvLoop.php

+            },
+            function ($signal) {
+                if ($this->signals->count($signal) === 0) {
+                    unset($this->signalEvents[$signal]);


This should probably stop() the watcher before clearing this reference, no?

Yes, it's very important to stop watcher before clearing the reference. Fixed.

clue · 2018-01-16T15:10:32Z

src/ExtEvLoop.php

+    private function getStreamListenerClosure($stream, callable $listener)
+    {
+        return function () use ($stream, $listener) {
+            call_user_func($listener, $stream, $this);


The additional $loop argument has been removed via #110, so the last parameter should be omitted here.

Thank you for pointing it! I removed this argument.

clue · 2018-01-16T15:11:57Z

src/Factory.php

+        } elseif (class_exists('EvLoop', false)) {
+            return new ExtEvLoop();
+        }
+        elseif (class_exists('EventBase', false)) {


Minor CS issue.

Fixed, thank you :)

clue · 2018-01-16T15:12:27Z

tests/AbstractLoopTest.php

        });

-        $this->assertRunSlowerThan(1.5);
+        $this->assertRunSlowerThan(2);


What is the motivation for this change?

I thought that there was a problem with floating point accuracy, but it was not. It should be changed to the previous 1.5 value. Fixed.

ivkalita · 2018-01-16T16:20:41Z

Also, I noticed that other loops implementations contains such code for signals callbacks:

function ($signal) {
    $this->signalEvents[$signal] = new SignalEvent($f = function () use ($signal, &$f) {
        $this->signals->call($signal);
        // Ensure there are two copies of the callable around until it has been executed.
        // For more information see: https://bugs.php.net/bug.php?id=62452
        // Only an issue for PHP 5, this hack can be removed once PHP 5 support has been dropped.
        $g = $f;
        $f = $g;
    }, $signal);
    $this->loop->add($this->signalEvents[$signal]);
},

My question is why using the $f identifier? Why not just

function ($signal) {
    $this->signalEvents[$signal] = new SignalEvent(function () use ($signal) {
        $this->signals->call($signal);
    }, $signal);
    $this->loop->add($this->signalEvents[$signal]);
}

WyriHaximus · 2018-01-16T16:36:14Z

@kaduev13 The issue in PHP 5.x is that the callable we're calling $this->signals->call($signal); can get cleaned up by the engine while we're still in it causing odd errors. By referencing $f and juggling it again $g we ensure the engine keeps it around for after the $this->signals->call($signal);.

ivkalita · 2018-01-16T16:54:55Z

@WyriHaximus, I checked that issue, but, as I understood, this bug is about aliasing. I mean, that this code will randomly fail:

function ($signal) {
    $this->signalEvents[$signal] = new SignalEvent($f = function () use ($signal, &$f) {
        $this->signals->call($signal);
    }, $signal);
    $this->loop->add($this->signalEvents[$signal]);
}

but this one should not (no alias == no problems?):

function ($signal) {
    $this->signalEvents[$signal] = new SignalEvent(function () use ($signal) {
        $this->signals->call($signal);
    }, $signal);
    $this->loop->add($this->signalEvents[$signal]);
}

WyriHaximus · 2018-01-16T17:02:37Z

@kaduev13 feel free to prove me wrong, I don't like the solution for that bug so if we can get rid of it that would be great 👍 . Also be sure to check signal example to be sure. IIRC this is one of the failing builds for that bug: https://travis-ci.org/reactphp/event-loop/jobs/258431073#L1054

ivkalita · 2018-01-17T09:31:13Z

@WyriHaximus, thank you! Unfortunately, the build url you provided is for commit, that does not exist. I'll try to reproduce this behavior.

WyriHaximus · 2018-01-17T21:31:43Z

@kaduev13 yes correct/sorry about that the commits where squashed just before merging to keep a clean commit log.

ivkalita · 2018-01-23T19:40:31Z

@WyriHaximus sorry, I didn't have enough time to completely test the issue with lambdas. I'll continue my investigation later. I added similar workaround to ExtEvLoop to ensure that everything will work well.
@clue, I fixed issues you have pointed at, review this changes again, please :)

clue · 2018-02-06T15:39:10Z

@kaduev13 Very good point about signal handling callbacks! I've just filed #150, once this is in, this should be simplified significantly (this will likely cause a merge conflict here though).

ivkalita · 2018-02-28T17:18:58Z

@clue nice work with signals, thank you! I rebased this branch onto master, review pls =)

ExtEvLoop implements event loop based on PECL ev extension.

WyriHaximus

LGTM

clue

@kaduev13 Again, thank you so much for keeping up with this and putting the effort into keeping this updated! 👍

Only this one minor remark, otherwise I'm happy to get this in finally! 🎉

clue · 2018-04-05T20:27:37Z

src/ExtEvLoop.php

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


This method has been removed from the public interface with #133, so this should probably be removed here.

clue · 2018-04-05T20:31:35Z

README.md


 This loop is known to work with PHP 5.4 through PHP 7+.

+#### ExtEvLoop


Should also be added to the TOC above 👍

ivkalita · 2018-04-06T15:42:57Z

@clue thank you for the review :) I fixed these issues.

clue

Awesome, thank you for the update! 🎉

WyriHaximus · 2018-04-08T17:41:36Z

Awesome, thank you 👍 !

ivkalita · 2018-04-09T08:53:30Z

Great to know that it's merged! Thank you!

WyriHaximus · 2018-04-09T12:03:05Z

@kaduev13 v0.5.1

WyriHaximus · 2018-04-16T22:28:15Z

@kaduev13 FYI I've been running it in production for a week now, as expected no issue what so ever. Well done 👍

WyriHaximus added this to the v0.5.1 milestone Jan 15, 2018

WyriHaximus requested review from WyriHaximus, clue and jsor January 15, 2018 20:40

WyriHaximus added new feature help wanted labels Jan 15, 2018

ivkalita force-pushed the ext-ev-loop branch 2 times, most recently from 405cb25 to 41018f5 Compare January 15, 2018 20:48

clue requested changes Jan 16, 2018

View reviewed changes

ivkalita force-pushed the ext-ev-loop branch 2 times, most recently from 97d946c to 8aef041 Compare January 16, 2018 16:04

ivkalita force-pushed the ext-ev-loop branch from 8aef041 to 38f8cf5 Compare January 23, 2018 19:35

clue changed the title ~~Implement ExtEvLoop.~~ Implement ExtEvLoop (PECL ext-ev) Feb 7, 2018

clue mentioned this pull request Feb 7, 2018

PeclEvLoop: Add new PeclEvLoop (PECL ext-ev) #97

Closed

This was referenced Feb 23, 2018

Support another EventLoop backend #152

Closed

Support lib ev easily php-pm/php-pm#356

Closed

ivkalita force-pushed the ext-ev-loop branch 2 times, most recently from 12d714f to 242fc11 Compare February 28, 2018 17:16

Implement ExtEvLoop.

b325a88

ExtEvLoop implements event loop based on PECL ev extension.

ivkalita force-pushed the ext-ev-loop branch from 242fc11 to b325a88 Compare February 28, 2018 17:39

Fix travis: skip ev extension installation if PHP version is 5.3.

43dc6ac

ivkalita force-pushed the ext-ev-loop branch from 52bbf89 to 43dc6ac Compare February 28, 2018 18:03

WyriHaximus approved these changes Apr 5, 2018

View reviewed changes

clue reviewed Apr 5, 2018

View reviewed changes

README.md

This loop is known to work with PHP 5.4 through PHP 7+.

#### ExtEvLoop

Copy link

Member

clue Apr 5, 2018

Choose a reason for hiding this comment

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

Should also be added to the TOC above 👍

ivkalita added 2 commits April 6, 2018 18:28

Remove ExtEvLoop::isTimerActive.

882597b

Fix README: add ExtEvLoop to TOC.

cedf941

ivkalita force-pushed the ext-ev-loop branch from ca19b0c to cedf941 Compare April 6, 2018 15:37

clue approved these changes Apr 8, 2018

View reviewed changes

jsor approved these changes Apr 8, 2018

View reviewed changes

WyriHaximus approved these changes Apr 8, 2018

View reviewed changes

WyriHaximus merged commit 5511b21 into reactphp:master Apr 8, 2018

clue mentioned this pull request May 9, 2018

Bump PHP dependency from 5.3.0 to 5.4.0 #165

Closed


		This loop is known to work with PHP 5.4 through PHP 7+.

		#### ExtEvLoop

Uh oh!

Conversation

ivkalita commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WyriHaximus commented Jan 15, 2018

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivkalita commented Jan 16, 2018

Uh oh!

WyriHaximus commented Jan 16, 2018

Uh oh!

ivkalita commented Jan 16, 2018

Uh oh!

WyriHaximus commented Jan 16, 2018

Uh oh!

ivkalita commented Jan 17, 2018

Uh oh!

WyriHaximus commented Jan 17, 2018

Uh oh!

ivkalita commented Jan 23, 2018

Uh oh!

clue commented Feb 6, 2018

Uh oh!

ivkalita commented Feb 28, 2018

Uh oh!

WyriHaximus left a comment

Choose a reason for hiding this comment

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivkalita commented Apr 6, 2018

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Apr 8, 2018

Uh oh!

ivkalita commented Apr 9, 2018

Uh oh!

WyriHaximus commented Apr 9, 2018

Uh oh!

WyriHaximus commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivkalita commented Jan 15, 2018 •

edited

Loading