Discourage libevent on PHP 7 #62

cboden · 2016-11-17T16:49:24Z

The libevent extension has not been updated for PHP 7 and I have seen reports of segfaults as a result.

clue · 2016-11-17T17:51:00Z

The docs (https://pecl.php.net/package/libevent) suggest libevent requires PHP < 6.0? How would one install libevent on PHP 7 in the first place?

Note there's also a PR that aims to add compatibility with PHP 7: php/pecl-event-libevent#2

I'm undecided if it makes sense to explicitly discourage libevent. I'm perfectly fine with lowering its priority and/or removing it from the Factory, as this still leaves users the choice 👍

cboden · 2016-11-21T21:01:25Z

I've seen a couple issues where users were able to install the extension on PHP 7 even though it's not officially supported yet and were complaining about segfaults.

This PR doesn't prevent people from using the extension as it only triggers a warning and the Factory will not select a loop that segfaults.

smalot · 2016-12-07T17:34:55Z

Any other feedback ? I'm looking for using PHP 7, so I'm too interested about this issue

clue · 2017-01-10T08:14:34Z

I've seen a couple issues where users were able to install the extension on PHP 7 even though it's not officially supported yet and were complaining about segfaults.

IMHO we should make this clear that we are not aware of any issues in React and this is likely an issue with the unofficial libevent bindings like so #55 (comment):

The libevent extension does not currently support PHP7. There's ongoing effort to update libevent to support PHP7 in php/pecl-event-libevent#2. However, the current version occasionally segfaults. This could not be observed in earlier version, so it is believed that this is a bug in libevent, rather than React.
As such, there's little we can do here. Discussion about this is probably best directed at #40.

And in the same vein then actively encourage users to check out recommended extensions for PHP 7 like so:

In the meantime, you may want to check out the "event" extension. We've seen reports of people successfully using this on PHP7.

cboden · 2017-01-10T15:43:53Z

What's the benefit of keeping factory order unchanged? I see a negative in segfaults but I don't see a positive to keeping it as-is.

clue · 2017-02-08T19:31:56Z

src/Factory.php

            return new LibEvLoop;
        } elseif (class_exists('EventBase', false)) {
            return new ExtEventLoop;
+        } elseif (function_exists('event_base_new') && version_compare(PHP_VERSION, '7.0.0', '<')) {
+            return new LibEventLoop();


Makes perfect sense to lower priority here and do not pick this extension on PHP 7+ at all 👍

clue · 2017-02-08T19:36:58Z

src/LibEventLoop.php

@@ -31,6 +31,10 @@ class LibEventLoop implements LoopInterface

    public function __construct()
    {
+        if (version_compare(PHP_VERSION, '7.0.0') >= 0) {
+            trigger_error('The libevent extension has not yet been updated for PHP 7, it is recommended you use a different loop', E_USER_WARNING);
+        }


I'm not a big fan of this tbqh 👎

With the above check in place we do not automatically create the LibEventLoop from our Factory. Afaict this is what most people use, so this should resolve this for the most part?

This means that if this constructor is invoked, it's explicitly been called. IMHO this is already advanced usage and we should not trigger a warning in this case.

Otherwise, how could a user possibly ignore this warning? Using @-suppression would result in zero output if the extension can not be loaded all.

Also, what happens when the extension will be updated to support PHP 7 again? We can't really detect this until we provide an update, in which case the above warning would be superfluous.

If the user has upgraded to PHP7 and somehow kept the extension installed, from the reports I've received, it's going to segfault. The trigger_error only issues a warning, the object will still be instantiated an be able to be used.

Also, what happens when the extension will be updated to support PHP 7 again?

It looks like that extension isn't going to be...but if it does we'll have to quickly update this to support it. Even if this happens the compare only issues a warning.

If the consensus is still a 👎 on this I'm good to remove this check in order to get the updated Factory order into the next release.

I'm not sure we had a consensus here, it was basically just my point of view :-) But if this error is removed, I'm all for getting this in! 👍

cboden · 2017-10-11T14:42:09Z

Updated. some > none 😀

We can discuss the warning in IRC or another issue.

clue · 2017-10-13T08:50:48Z

src/Factory.php

            return new LibEvLoop;
        } elseif (class_exists('EventBase', false)) {
            return new ExtEventLoop;
+        } elseif (function_exists('event_base_new') && version_compare(PHP_VERSION, '7.0.0', '<')) {


Will update this to use PHP_VERSION_ID instead

clue

Thanks for the update, I've rebased this to fix the failing unit tests and LGTM!

WyriHaximus

LGTM

cboden assigned clue Nov 17, 2016

clue added help wanted maintenance new feature labels Jan 10, 2017

clue requested changes Feb 8, 2017

View reviewed changes

clue mentioned this pull request Aug 3, 2017

Segmentation Fault ratchetphp/Pawl#51

Closed

cboden added this to the v0.5.0 milestone Oct 11, 2017

jsor approved these changes Oct 11, 2017

View reviewed changes

clue reviewed Oct 13, 2017

View reviewed changes

cboden added 2 commits October 13, 2017 10:51

Discourage libevent on PHP 7

bda850f

Remove warning on invalid version mismatch

f1c4746

clue force-pushed the no7-libevent branch from 10b71ea to f1c4746 Compare October 13, 2017 08:54

clue approved these changes Oct 13, 2017

View reviewed changes

WyriHaximus approved these changes Oct 13, 2017

View reviewed changes

WyriHaximus merged commit f0117e4 into master Oct 13, 2017

cboden deleted the no7-libevent branch October 13, 2017 15:07

clue mentioned this pull request Oct 13, 2017

Make signal handling tests more robust and increase test timeouts #111

Merged

clue mentioned this pull request Feb 2, 2024

Drop deprecated ExtLibeventLoop and ExtLibevLoop (PHP 5 only) #273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discourage libevent on PHP 7 #62

Discourage libevent on PHP 7 #62

cboden commented Nov 17, 2016 •

edited

Loading

clue commented Nov 17, 2016

cboden commented Nov 21, 2016

smalot commented Dec 7, 2016

clue commented Jan 10, 2017

cboden commented Jan 10, 2017

clue Feb 8, 2017

clue Feb 8, 2017

cboden Oct 10, 2017

cboden Oct 10, 2017

clue Oct 10, 2017

cboden commented Oct 11, 2017

clue Oct 13, 2017

clue left a comment

WyriHaximus left a comment

Discourage libevent on PHP 7 #62

Discourage libevent on PHP 7 #62

Conversation

cboden commented Nov 17, 2016 • edited Loading

clue commented Nov 17, 2016

cboden commented Nov 21, 2016

smalot commented Dec 7, 2016

clue commented Jan 10, 2017

cboden commented Jan 10, 2017

clue Feb 8, 2017

Choose a reason for hiding this comment

clue Feb 8, 2017

Choose a reason for hiding this comment

cboden Oct 10, 2017

Choose a reason for hiding this comment

cboden Oct 10, 2017

Choose a reason for hiding this comment

clue Oct 10, 2017

Choose a reason for hiding this comment

cboden commented Oct 11, 2017

clue Oct 13, 2017

Choose a reason for hiding this comment

clue left a comment

Choose a reason for hiding this comment

WyriHaximus left a comment

Choose a reason for hiding this comment

cboden commented Nov 17, 2016 •

edited

Loading