Suggestion: use protocol-fcgi library for FCGI handling

[lisachenko]

Hello, just noticed your daemon and liked it, because it has many common with things that I do (private repo for now). I have an OOP library for working with FCGI: https://github.com/lisachenko/protocol-fcgi and think that it can be used for you needs as a low-level transport implementation.

What do you think about this? Thanks!

[AndrewCarterUK]

Hello!

Glad to hear that you like it!

In one of the earlier versions of the library I actually had an implementation of the protocol that was much more similar to yours, but I dropped it for performance reasons. Although it produced much cleaner and more understandable code, the creation of an object for each record had a performance penalty compared to reading and writing directly from a stream. As a result the library is faster, but the ConnectionHandler class is less tidy and harder to maintain!

You can actually view the old implementation using record classes here: https://github.com/PHPFastCGI/FastCGIDaemon/tree/18d49986eb76ce7696655da0a4cf47fd3900d0ae/src/Record

All that said, the last version of the library supports different drivers. So it would be very easy to create a driver that used your implementation of the protocol which we could then compare alongside the current core driver. If it passes all the tests then I'd be happy to provide it as a driver option. I would recommend that you develop some unit tests for your library though - you can set up Travis CI for free to run your tests before you merge changes to your library (helps to stop breaking things!).

I'm a bit stuck for development time at the moment, as I'm currently working on the php extension and getting it properly tested. But you're more than welcome to start a fork and pull request for a driver that uses your implementation and I'll be able to give you a hand when I get some time free :)

Interested to hear your thoughts,

Andrew

[lisachenko]

Thanks for the quick response!

I understood your concerns about performance of OOP version for FCGI parser. However, on a high load from FCGI queries with big list of params, memory allocation for objects will be better and faster (see http://jpauli.github.io/2015/03/24/zoom-on-php-objects.html#starter-example for explanation why this happens). Of course, there will be some penalty, but it can be relatively small to the whole request handling, need to test this fact.

Your suggestion regarding tests in my library and travis-ci handling is accepted. However, my library is a pure implementation of FCGI protocol, nothing more, so tests will be pretty simple: take any valid FCGI record, parse it and check it with expected result and vice versa for encoding.

Just hadn't time early for that and want to use it internally only for my daemon needs :smiley:

I will think about providing a PR with driver if speed of my library will be good enough for you needs.

[AndrewCarterUK]

The speed of the driver wouldn't prevent me from including it as an option - I was just saying why I originally switched from that approach. I load tested the two versions with the 'ab' tool and got a good performance improvement from the messy version so I went with it.

That could also be due to my specific implementation however - it could well be that there is little performance difference between OOP and not when done properly! As I said though, even if it was slower it wouldn't stop me from including it.

And good news on the tests - as you say it should be quite easy. It just makes it easier to list as a dependency if there is protection against things breaking on bug fixes :)

[lisachenko]

Status update: released version 1.1.0 of lisachenko/protocol-fcgi, full code coverage, travis-ci, scrutinizer and SensioInsight monitoring were enabled for project. Also added several badges https://github.com/lisachenko/protocol-fcgi

Now it's look production ready )

[AndrewCarterUK]

Very nice, good work!

After I've finished the work on the core extension I'll create a branch using your implementation of the FastCGI protocol. By the looks of things I'll only need to change the 'ConnectionHandler' class.

[lisachenko]

Expired :)