Unleash / unleash-client-php-wip

PHP client for Unleash
Apache License 2.0
11 stars 14 forks source link

settimeout problems #5

Open MyDigitalLife opened 5 years ago

MyDigitalLife commented 5 years ago

So when working to get this up and running i found a problem. All implementations are sending metrics periodically like in the nodejs app:

this.timer = setTimeout(() => {
    this.sendMetrics();
}, this.metricsInterval);

The problem? PHP has no set timeout function or really anything that's comparable in its default API. There are some solutions that allow this but they will need to add PHP extensions.

The external library that I found that might be able to provide us with this functionality is something like libevent

Will be able to implement is like explained here: http://php.net/manual/en/function.event-add.php

I would love to hear some opinions. I'm trying to keep the client to have as few dependencies as possible but if we need this it will make adoption harder. Check out my current branch for my progress: https://github.com/MyDigitalLife/unleash-client-php/tree/client/src

@ivarconr

ivarconr commented 5 years ago

There was similar challenges in java. The solution I went for was to spin up a separate dedicated thread to do all the background work (fetch toggles, metrics etc).

I agree to holding external dependencies to minimum!

Is it not possible to spin up child processes in PHP?

What about: http://www.php.net/manual/en/function.pcntl-fork.php

Also https://github.com/cocur/background-process looks promising.

MyDigitalLife commented 5 years ago

Looks like a solution without adding a extension dependency is not a option. pnctl_fork also required a extension. I think I'll come back to this at a later time. I'll write the code without the a threaded functionality for now and when all the other work is done choose the best solution for this problem.

I did already try out pcntl_fork and its easy to install in the default docker-image but wasn't really doing what i was hoping, I basically got endless loops. Looks like the main thread does wait for the child threads to close, and I don't see a way to send a easy kill command. But i might just need to put some more time into it.

dungahk commented 5 years ago

What about this class?

http://php.net/manual/en/class.evtimer.php

MyDigitalLife commented 5 years ago

@dungahk Looks like that's going to work.

From what i can tell every option to fix this will require a extension, some easier to add then others. For now I'll try it with the EV extension and if we find a better alternative we can always use that.

llwp commented 5 years ago

Can I challenge a base assumption here? Why do we need time based metrics as a requirement? Granted, it can be a feature available if the dependencies are acceptable etc etc ... But given event based metrics, and all other functionality.. should this actually be a requirement?

MyDigitalLife commented 5 years ago

Yeah, that's a really good question. I haven't been able to get this working and given the dependencies it adds I do think it might be a bit overkill. But that off course depends on the minimal functionality Unleash expects. @ivarconr what do you think?

ivarconr commented 5 years ago

I agree that metrics can be an optional extension. It would be nice to support this of course.

One would still find a suitable way to solve caching features toggles though. What would be the recommended way to facilitate caching in PHP?

I have been looking in to how launchdarkly php-client is implemented. Launchdarkly requires a cache middleware or a redis-backed caching, and is this to much to ask for?

kaystrobach commented 5 years ago

What you think?

If you want to have the toogles always fetched use a null backend for caching and fetch the data directly.

kaystrobach commented 5 years ago

btw. you can then use any of these packages to have the real cache:

this also makes it flexible and easy to integrate the package into symfony / flow / typo3 / wordpress / whatever as the package can stick to the native caching of the used framework

llwp commented 5 years ago

Very much like the direction - ease of integration to known frameworks and symfony in particular is a big win.

kaystrobach commented 5 years ago

@MyDigitalLife IMHO the approch in php is very different to node.

While you start a thread in node, which keeps running - php will have a new thread per request! This means, there is generally no need to have setTimeout or similar functions in php.

Additionally this also means, that you need cron or other triggers to update information in the background.

I would really love to get some feedback from you and how to help here.

kaystrobach commented 5 years ago

just to complete the list:

to get the keys you can use symfony console and define one command. if the whole fetching is somehow encapsulated the command can all the service and collect all the needed stuff.

https://symfony.com/doc/current/components/console.html

And you get all the fancy stuff for free ... (cli tables, dialogs, nice display of exceptions, verbosity ...)