Run externals.io on ReactPHP to improve performances #48

mnapoli · 2017-07-23T13:27:41Z

This PR introduces ReactPHP in order to replace PHP-FPM.

The web server is handled by React as a long running PHP application. As a result response time are drastically reduced.

Also, this was fun.

Profiling

Home page: https://blackfire.io/profiles/compare/7b69472b-21f5-4567-b863-0025961ffdbf/graph

News page: https://blackfire.io/profiles/compare/341906f5-6b66-467a-92ef-5febe00e1eda/graph

Demo

The branch is deployed thanks to Platform.sh: https://react-e5mxmca-2snxbat5vggso.eu.platform.sh/

…marks"" This reverts commit bc0100f.

Needs to be cleaner later.

mnapoli · 2017-07-23T13:30:33Z

.platform.app.yaml

-            root: 'web'
-            # The front-controller script which determines where to send
-            # non-static requests.
-            passthru: '/index.php'


This is how we move from a PHP-FPM setup to a ReactPHP setup on Platform.sh (documentation: https://docs.platform.sh/languages/php.html#alternate-start-commands)

You probably still want the locations block, so that static assets are served as you expect. Just replace passthru: '/index.php' with passthru: true (once your application is talking HTTP directly, it has no concept of script anymore).

mnapoli · 2017-07-23T13:30:57Z

.platform.app.yaml


 crons:
    # Look for new emails every 5 minutes.
-    drush-queue:
+    sync:


I simply renamed the job for clarity, this should not have been included in this PR, sorry for the noise.

mnapoli · 2017-07-23T13:32:15Z

console

+    });
+
+    $loop->run();
+});


The ./console serve command runs the React webserver.

Since React supports PSR-7 this is extremely easy to integrate with Stratify or any other PSR-7 framework.

Be aware that it's not really compatible. https://github.com/reactphp/http/blob/d1ee39ad2a1f5cf3b5f727cdd14f466816b82bba/src/HttpBodyStream.php

As documented thoroughly: http://reactphp.org/http/#request

Thanks, from what I've read this should not be an issue here but I learned something!

mnapoli · 2017-07-23T13:32:31Z

console

+
+    $server = new Server(function (ServerRequestInterface $request) use ($http, $db) : ResponseInterface {
+        // Reset the DB connection
+        $db->close();


I should move this into a middleware.

This line resets the database connection on every HTTP request. This is necessary because database connections can break or timeout, and Doctrine will not reconnect automatically. This is a problem specific to long running applications (like ReactPHP), on a classic application the PHP process dies (with the DB connection) on every HTTP request so there is no need for that.

I found this analysis of the issue:

http://brady.lucidgene.com/articles/pdo-lost-mysql-connection-error

There are some ideas for other approaches in the comment section...

Very interesting link thank you! I was starting to consider whether to abstract DBAL behind another abstraction that restarts connections automatically… But that's overkill I think, and I don't think DBAL has an interface for the Connection…

https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/Connection.php

I don't know why but in several projects I tried to use this interface but always ended up using Doctrine\DBAL\Connection directly instead.

mnapoli · 2017-07-23T13:33:07Z

res/config/config.php

+    'path.public' => __DIR__ . '/../../web',
+
+    // Platform.sh defines this variable
+    'http.port' => env('PORT', 8000),


By default the application will run on port 8000 (e.g. in dev environment when running make preview).

mnapoli · 2017-07-23T13:34:24Z

res/http.php

@@ -27,8 +30,11 @@
 * HTTP stack.
 */
 return pipe([
+    BlackfireMiddleware::class,
+    NewRelicMiddleware::class,


Since Blackfire and New Relic consider that 1 HTTP request == 1 PHP process, they will not work correctly out of the box.

Those 2 middlewares activate manually Blackfire and New Relic for each HTTP request.

Those 2 middlewares could be moved to their own open source projects though.

mnapoli · 2017-07-23T13:36:38Z

res/http.php

    MaintenanceMiddleware::class,
    ErrorHandlerMiddleware::class,
+    AssetsMiddleware::class,


This middleware is useful only in dev environment. It serves static files (aka assets) like CSS, JS, etc.

Previously we didn't need it because PHP's built-in webserver (php -S 127.0.0.1:8000) was handling it automatically. But since we are using React as the webserver, we need to serve the static files.

Using a middleware is not really efficient performance-wise, but this middleware is only serving files in dev environment, not in production.

In production the middleware will not do much work, still it could be optimized. However I don't really care, the application is super duper fast, that's maybe something to optimize later if we really care.

mnapoli · 2017-07-23T13:37:02Z

res/http.php

@@ -141,6 +147,7 @@

        // Keep backward compatibility with old URLs (old threads)
        '/thread/{id}' => route(function (int $id, EmailRepository $emailRepository, Connection $db) {
+            newrelic_name_transaction('thread_legacy');


This should have been committed on master too, my mistake.

mnapoli · 2017-07-23T13:38:13Z

Makefile

@@ -1,5 +1,5 @@
 preview:
-	ENV=dev php -S localhost:8000 -t web
+	ENV=dev ./console serve


make preview is the command used in development environment to run a small webserver.

We were previously using PHP's built-in webserver, now we simply launch the React application.

staabm · 2017-07-23T13:42:22Z

I would suggest to use amphp/aerys instead which als supports websockets etc. But still this is kinda nice!

mnapoli · 2017-07-23T13:44:27Z

@staabm thanks, but we do not need websockets for now? I went for the most popular implementation that could work on Platform to be honest :)

ratibus · 2017-07-23T17:35:55Z

src/Application/Middleware/AssetsMiddleware.php

+        $body = new Stream($file);
+
+        $array = explode('.', $file);
+        $extension = strtolower(array_pop($array));


Use pathinfo ?

Nice! Thanks!

staabm · 2017-07-23T17:49:53Z

src/Application/Middleware/AssetsMiddleware.php

+    {
+        $path = $request->getUri()->getPath();
+
+        $file = $this->publicPath . $path;


You need to make sure that te resulting path is within your webroot. Otherwise one can read arbritrary files..

Will do, I implemented this quickly for dev environments but you are right it's better to secure properly.

It's probably best not to implement your own, e.g. Aerys has that built-in. Not sure whether ReactPHP's server has something like that yet.

staabm · 2017-07-23T17:51:47Z

src/Application/Middleware/AssetsMiddleware.php

+
+        $array = explode('.', $file);
+        $extension = strtolower(array_pop($array));
+        $mimeType = self::MIME_TYPES[$extension] ?? 'application/octet-stream';


Dynamically determine the mimetype instead of your custom map?

http://php.net/manual/en/function.mime-content-type.php

I wanted a fast response time because for most assets this will work. Maybe that could be a fallback to the array solution.

This is good enough, Aerys does the same thing.

staabm · 2017-07-23T20:09:12Z

src/Application/Middleware/AssetsMiddleware.php

+        $extension = strtolower(array_pop($array));
+        $mimeType = self::MIME_TYPES[$extension] ?? 'application/octet-stream';
+
+        return new Response($body, 200, [


Should this support cache revalidation with http 304 or similar?

Not really important when it's only used for development.

staabm · 2017-07-23T20:11:11Z

composer.json

-        "sentry/sentry": "^1.7"
+        "sentry/sentry": "^1.7",
+        "react/http": "^0.7.2",
+        "blackfire/php-sdk": "^1.8"


Afair you only need this sdk when the extension is not loaded.

You could also do a extension_loaded check before registering the middleware and drop this dependency

mindplay-dk · 2018-09-14T16:31:57Z

@mnapoli did you intentionally close this? if it didn't work out, I'd love to hear why :-)

staabm · 2018-09-14T16:42:38Z

closed because of #90

WyriHaximus · 2018-09-14T16:48:13Z

@staabm thanks, was also interested in the why 👍

mnapoli · 2018-09-14T16:58:42Z

Right sorry for not leaving a comment ^^ Testing React was fun. This PR could be reopened later. But I want to leave platform.sh builds available for other PRs. And in the future (hopefully soon) I want to try and move externals.io to serverless/aws lambda using Bref. There is no strong reason except testing and learning so don’t expect any gains. In any case that wouldn’t work with React’s execution model.

mindplay-dk · 2018-09-15T08:58:28Z

@mnapoli as long as you're just messin' around, I recommend trying out Swoole as well - it's neat :-)

mnapoli · 2018-09-15T12:21:40Z

Right I've been wanting to look at it, it's very promising!

Ping @Nyholm the diff of this PR might interest you ;)

mnapoli added 9 commits July 17, 2017 21:11

Run the web application with React

c341a0c

Enable Blackfire support with React

54ee17c

Revert "Try enabling Blackfire sooner for more accurate benchmarks"

bc0100f

Use the native event extension for better performances

c2915e8

Revert "Revert "Try enabling Blackfire sooner for more accurate bench…

e12ab04

…marks"" This reverts commit bc0100f.

Fix unused import

b67cb6e

Use the native event extension for better performances

de8bc40

Enable New Relic support with React

92bf137

Fix DB connection broken

aeab177

Needs to be cleaner later.

mnapoli commented Jul 23, 2017

View reviewed changes

ratibus reviewed Jul 23, 2017

View reviewed changes

staabm reviewed Jul 23, 2017

View reviewed changes

mnapoli closed this Sep 14, 2018

Run externals.io on ReactPHP to improve performances #48

Run externals.io on ReactPHP to improve performances #48

Conversation

mnapoli commented Jul 23, 2017

Profiling

Demo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnapoli Jul 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Jul 23, 2017 • edited Loading

mnapoli commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mindplay-dk commented Sep 14, 2018

staabm commented Sep 14, 2018

WyriHaximus commented Sep 14, 2018

mnapoli commented Sep 14, 2018

mindplay-dk commented Sep 15, 2018

mnapoli commented Sep 15, 2018 • edited Loading

mnapoli Jul 23, 2017 •

edited

Loading

staabm commented Jul 23, 2017 •

edited

Loading

mnapoli commented Sep 15, 2018 •

edited

Loading