Feature Request: Allow Multiple Network Connections for a Client

[mikesoule]

I'm using this library to build a health checker for a Kubernetes pod with multiple PHP containers. I want a single health check that calls a health check in each of my PHP containers. If your familiar with Google Kubernetes Engine, this is to allow the health check endpoint for a load balancer backend to check the health of multiple PHP-FPM containers within a pod.

Expected Behavior

A Client should allow requests to multiple PHP-FPM endpoints such that methods like Client:: hasUnhandledResponses() and Client:: readReadyResponses() are managing all requests/connections.

Actual Behavior

Currently, a Client instance is bound to a single connection/PHP-FPM endpoint.

Further comments

@hollodotme If I build this and issue a pull request, would you be interested in incorporating this feature? I think it would involve the option of setting the connection on the request vs the client. If you're open to it, please let me know if you have a preference for how it should be implemented.

[hollodotme]

@mikesoule Thanks for the proposal. I'm only on mobile, so the following are just short initial thoughts for discussion. :)

Have you seen https://github.com/hollodotme/fast-cgi-proxy ?

I'd say this feature could probably be a better fit over there. It needs some care anyway. 😇 I think it'd "just" need another distribution mechanism (currently random and round robin) to make it work. Not sure though.

Moving the connection to the request would mean a lot of rework and BC breaks, I guess at first glance. But maybe an additional possibility to override the client's connection in the request could do the trick already. 🤔

I'm always open and thankful for contributions and PRs. If you have some time I'd be happy to discuss a prototype you come up with just to settle the general feature scope and agree on full implementation. What do you think?

[mikesoule]

@hollodotme I had not seen the fast-cgi-proxy repo, not sure how I missed that! I'll look that over and consider my current use case, then I'll get back to you in a couple days. Thanks!

[hollodotme]

@mikesoule I have implemented a solution for cluster requests and cluster status (that should cover exactly your use-case) in hollodotme/fast-cgi-proxy#4 and released v0.2.0.

Please have a look at the example code for cluster status.

[mikesoule]

Thanks @hollodotme, not exactly what I was looking for but still very useful!

Here's the specific problem I was trying to solve.

One of my clients has several PHP microservices, each in their own container, running in one pod on a Google Kubernetes cluster. Each service has its own custom health check endpoint that verifies the service is up and has all its dependencies (DB connection etc.). Since the Google load balancer health checks can only be sent to one of the containers in the cluster, I needed to create a new endpoint that would in-turn hit all the other endpoints and aggregate their responses into one global health check.

What I ended up doing is just creating various FastCGI\Client instances and then calling waitForResponses() on each one. My original idea was to be able to just use one client with several NetworkSocket instances. This seems to work well but if there's a more efficient way to do it, please let me know.

Note to anyone else who implements an aggregate health check like this on GKE:

Google sends an almost unreasonable number of health checks. It's wise to use a cache tool, like APC, to return cached success responses for 2 to 5 seconds after each successful health check. Otherwise the health checks may start to run over themselves (the next one starts before the last finishes). Don't cache failed checks or one failed check will cause the load balancer to think your service is completely down.

[hollodotme]

Hello @mikesoule, thank you for the more detailed explanation. I admit that the current (cluster) proxy implementation is not the most efficient one, as it uses multiple stream_select() calls (one for each internal client). The new [cluster requests](https://github.com/hollodotme/fast-cgi-proxy#cluster-requests functionality) basically is an abstraction of what you did manually as described above.

I obviously misunderstood that you want to query the php-fpm status of each container and implemented that use-case in addition on top of cluster requests as Cluster status. :)

In order to use a more efficient way under the hood, I see the following options at the moment:

1. In hollodotme/fast-cgi-proxy: Implement the method that executes the stream_select() call (and all necessary handlers) directly in the Proxy class instead of using multiple clients. This way all sockets could be checked with one stream_select() call. Basically a slightly different implementation of the Client, using multiple connections instead of one. This could be done without changing the current public API of the proxy classes.

2. In hollodotme/fast-cgi-client: Move the connection to the sendRequest()/sendAsyncRequest() methods in the Client implementation, so each request must be explicitly sent to a certain connection. That would be a major BC break, but IMO the most flexible solution. (As I'm already working on version 3.0, this BC break could go in there).

# Always use the same connection

$client = new Client();
$client->sendAsyncRequest($connection, $request1);
$client->sendAsyncRequest($connection, $request2);
$client->sendAsyncRequest($connection, $request3);

# Use different connections for each request

$client = new Client();
$client->sendAsyncRequest($connection1, $request1);
$client->sendAsyncRequest($connection2, $request2);
$client->sendAsyncRequest($connection3, $request3);

The fast-cgi-proxy classes would also immediately gain profit from this implementation, as only one client would be needed internally and option 1 would become obsolete.

I also considered optional override of the default connection per request, but that leads to confusing code, IMO.

$client = new Client($connecton1);
$client->sendAsyncRequest($request1);
$client->sendAsyncRequest($request2, $connection2);
$client->sendAsyncRequest($request3, $connection3);

The $connection1 looks misplaced in constructor and the user needs to know that this is the default connection that is used for the first call of sendAsyncRequest().

I also considered adding the connection to the request object, but that could (depending on implementation) lead to more unnecessary code for the user:

$client = new Client();

$request1->setConnection($connection1);
$client->sendAsyncRequest($request1);

$request2->setConnection($connection2);
$client->sendAsyncRequest($request2);

$request3->setConnection($connection3);
$client->sendAsyncRequest($request3);

This would also mix semantically different objects - request payload and request transport.

---

So for now Option 2 is my favorite, as it is very explicit, flexible and would also be beneficial for the proxy repository. What do you think?

[mikesoule]

@hollodotme I like option 2 as well. If you wanted to avoid having to pass the connection with each request, you could take the approach of allowing the request to optionally include the full address and then, in cases where you can't determine whether it's a socket or network connection, assume socket. Users familiar with MySQL and GuzzleHttp will be accustomed to these conventions though I'm not sure that's a better approach. Each idea seems to have pros and cons. Given a choice between one or two fewer method calls for the user vs a more explicit, clear interface, I agree with you that explicit and easy to understand is better.