Keep state between processes #195

rayrutjes · 2016-12-10T15:44:50Z

We abstracted an interface for the failed hosts retrieval so that users can easily choose their caching strategy based on what is best according to their environment.

By default we push the new File Based cache strategy, but if the tmp directory is not writable, we fallback to the in memory strategy.

User can provide an implementation of FailingHostsCache as a replacement of this strategy.

Because of the current design of the Client, the FailingHostsCache implementation needs to be provided at Client initialization because that is where the ClientContext is instantiated and during that instantiation we need to rotate the hosts.

Regarding the BC static to non static, given it has only been available in 1 version and was only used internally + wasn't even called statically, I think we are safe doing the change.

TODO:

Unit tests
Choose if we push the file based cache by default or if we keep the in memory strategy
Implement in memory cache invalidation for long running PHP processes > 5min
See what we do about the static to public method signature of addFailingHost (BC) that was only introduced in last version

maxiloc · 2016-12-12T22:17:42Z

is it expected that the test are not passing with hhvm ?

maxiloc · 2016-12-12T22:22:38Z

Good job on this. Looks good.
Still have some doubt on file first but except that we're good to go

rayrutjes · 2016-12-12T22:33:09Z

I don't expect the tests to fail. I will make sure they pass on HHVM.
I see were it broke the other tests. Should be an easy fix.

rayrutjes · 2016-12-12T22:42:42Z

I'm afraid it is an HHVM limitation: https://gist.github.com/tlevi/8503668
I will look for a workaround.

rayrutjes · 2016-12-12T22:46:49Z

Here is a workaround: https://github.com/LExpress/symfony1/pull/127/files
Will see if there are other cleaner solutions.

Smarty: smarty-php/smarty@f9d9ca0#diff-07258ab0fdc58bf99abe45a5f24a8aceR21

rayrutjes · 2017-01-05T14:10:15Z

What's left to be done:

I will revert to in memory by default. We never know about the side-effects of the file based caching strategy and I don't want to take the risk to break existing implementations.
I will more thoroughly test the file based strategy

maxiloc · 2017-02-06T10:09:25Z

Good for me.
We'll probably need to mention this in the doc

JanPetr · 2017-02-06T15:20:27Z

I'm really not sure about eval() in the file cache. Why did you decide to go this way?
eval() is not supported on vast majority of hostings and is considered to be security vulnerability.

Why not just use text file with specific syntax which is easy to parse? The code will be cleaner, easier to read, we won't have to deal with OPCache and HHVM workaround.

rayrutjes · 2017-02-06T15:30:13Z

eval is only used in HHVM because of this limitation: facebook/hhvm#1447

And the files are stored as PHP files to take advantage of PHP opcache which makes the read even faster than other caching strategies.
Here is the inspiration: https://blog.graphiq.com/500x-faster-caching-than-redis-memcache-apc-in-php-hhvm-dcd26e8447ad

If we go without opcache the code will be cleaner but it will be that much slower than the current approach. It will still be better than nothing though.

JanPetr · 2017-02-06T15:46:10Z

My bad about the eval() and HHVM workaround.
Makes sense -> #millisecondsmatters

And how about checking in assertCacheFileIsValid if HHVM && eval() is enabled?

rayrutjes · 2017-02-06T15:50:17Z

You are right, we are missing some edge cases there that we should probably cover, nice catch @JanPetr ❤️

JanPetr

I just noticed some small glitches :)
Good choice with JSON file, much better then PHP file and eval() for me.

JanPetr · 2017-03-29T09:07:33Z

src/AlgoliaSearch/FileFailingHostsCache.php

+     */
+    public function __construct($ttl = null, $file = null)
+    {
+        $this->failingHostsCacheFile = null === $file ? $this->getDefaultCacheFile() : (string) $file;


I'd rewrite this line to be more explicit? Like that it's not really readable and I have to really focus to get what it does :)

JanPetr · 2017-03-29T09:11:52Z

src/AlgoliaSearch/FileFailingHostsCache.php

+        if ($json === false) {
+            return;
+        }
+        @file_put_contents($this->failingHostsCacheFile, $json);


Is the @ really needed? All conditions are checked when the class is created and by suppressing error here we might lose some valuable info for potential debugging.

file_put_contents throws warning, not fatal - worth to remove @

JanPetr · 2017-03-29T09:25:07Z

src/AlgoliaSearch/FileFailingHostsCache.php

+
+        $this->assertCacheFileIsValid($this->failingHostsCacheFile);
+
+        if (null === $ttl) {


Change to $var === null because of consistency.

JanPetr · 2017-03-29T09:25:18Z

src/AlgoliaSearch/ClientContext.php

@@ -129,6 +126,18 @@ public function __construct($applicationID, $apiKey, $hostsArray, $placesEnabled
        $this->algoliaUserToken = null;
        $this->rateLimitAPIKey = null;
        $this->headers = array();
+
+        if (null === $failingHostsCache) {


Change to $var === null because of consistency

JanPetr · 2017-03-29T09:25:30Z

src/AlgoliaSearch/InMemoryFailingHostsCache.php

+     */
+    public function __construct($ttl = null)
+    {
+        if (null === $ttl) {


Change to $var === null because of consistency

JanPetr · 2017-03-29T09:25:38Z

src/AlgoliaSearch/InMemoryFailingHostsCache.php

+            // Keep a local cache of failed hosts in case the file based strategy doesn't work out.
+            self::$failingHosts[] = $host;
+
+            if (null === self::$timestamp) {


Change to $var === null because of consistency

JanPetr · 2017-03-29T09:25:45Z

src/AlgoliaSearch/InMemoryFailingHostsCache.php

+     */
+    public function getFailingHosts()
+    {
+        if (null === self::$timestamp) {


Change to $var === null because of consistency

JanPetr · 2017-03-29T09:25:57Z

src/AlgoliaSearch/FileFailingHostsCache.php

+    private function loadFailingHostsCacheFromDisk()
+    {
+        $json = @file_get_contents($this->failingHostsCacheFile);
+        if (false === $json) {


Change to $var === false because of consistency

JanPetr · 2017-03-29T09:31:47Z

src/AlgoliaSearch/FileFailingHostsCache.php

+     */
+    private function loadFailingHostsCacheFromDisk()
+    {
+        $json = @file_get_contents($this->failingHostsCacheFile);


file_get_contents throws warning, not fatal - worth to remove @ in order not to loose debugging information

This PR aims to resolve the issue that PHP shares no state between PHP processes. If the primary DNS server is down, then every new PHP process would have to hit the timeout connection once before falling back to the other DNS provider. For now users have to manually state that they want to use the file based caching strategy. After the robustness of this has been proven, we will make this the default strategy. Note that if you are working in a RO filesystem, it would automatically fallback to in memory.

rayrutjes · 2017-03-29T13:30:31Z

Looks like this problem is related to sebastianbergmann/phpunit#1684
I referenced the local PHPUnit binary. Let's see how that works out.

maxiloc · 2017-03-29T13:48:25Z

src/AlgoliaSearch/FileFailingHostsCache.php

+            return array();
+        }
+
+        $json = file_get_contents($this->failingHostsCacheFile);


I think you need the @ here

@JanPetr made a point on this subject. We should not obfuscate errors here. The errors here are only warnings so it makes sense to have them to be able to know what happens.

maxiloc · 2017-03-29T13:48:57Z

src/AlgoliaSearch/FileFailingHostsCache.php

+        $fileDirectory = dirname($file);
+
+        if (! is_writable($fileDirectory)) {
+            throw new \RuntimeException(sprintf('Cache file directory "%s" is not writable.', $fileDirectory));


I am not sure we want to throw non catched exceptions here

Yes we do, this strategy makes non sense if the infra does not allow it. This is exactly what Runtime exceptions are for ;)

If tomorrow we push this strategy by default, we will catch the exception and instantiate a fallback instead.

maxiloc · 2017-03-29T13:50:04Z

src/AlgoliaSearch/FileFailingHostsCache.php

+    {
+        $json = json_encode($data);
+        if ($json !== false) {
+            file_put_contents($this->failingHostsCacheFile, $json);


Same here we probably need the @

Same as before ;)

rayrutjes · 2017-03-29T14:30:27Z

All good @JanPetr @maxiloc ?
We should then add a line to the docs.

JanPetr · 2017-03-29T14:35:21Z

Good for me

rayrutjes · 2017-03-29T14:48:06Z

Will merge and release on Monday.

rayrutjes added the wip label Dec 10, 2016

rayrutjes self-assigned this Dec 10, 2016

rayrutjes force-pushed the keep-state-between-processes branch 5 times, most recently from e014b59 to 62dd14b Compare December 11, 2016 21:57

rayrutjes changed the title ~~[POC] Keep state between processes~~ Keep state between processes Dec 11, 2016

rayrutjes requested review from maxiloc and JanPetr December 11, 2016 22:04

rayrutjes added ready and removed wip labels Dec 11, 2016

rayrutjes added wip and removed ready labels Jan 5, 2017

rayrutjes force-pushed the keep-state-between-processes branch from 42dc56f to cfa7113 Compare January 5, 2017 11:29

rayrutjes force-pushed the keep-state-between-processes branch 2 times, most recently from 770d78e to 4cea7d7 Compare March 27, 2017 15:28

JanPetr suggested changes Mar 29, 2017

View reviewed changes

rayrutjes force-pushed the keep-state-between-processes branch from 99253db to dabc90c Compare March 29, 2017 11:15

JanPetr approved these changes Mar 29, 2017

View reviewed changes

fix(HHVM): try to use local phpunit version

af56beb

maxiloc reviewed Mar 29, 2017

View reviewed changes

rayrutjes added ready and removed wip labels Mar 29, 2017

rayrutjes merged commit 86fecf7 into master Apr 3, 2017

rayrutjes deleted the keep-state-between-processes branch April 3, 2017 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep state between processes #195

Keep state between processes #195

rayrutjes commented Dec 10, 2016 •

edited

Loading

maxiloc commented Dec 12, 2016

maxiloc commented Dec 12, 2016

rayrutjes commented Dec 12, 2016

rayrutjes commented Dec 12, 2016

rayrutjes commented Dec 12, 2016 •

edited

Loading

rayrutjes commented Jan 5, 2017 •

edited

Loading

maxiloc commented Feb 6, 2017

JanPetr commented Feb 6, 2017

rayrutjes commented Feb 6, 2017

JanPetr commented Feb 6, 2017

rayrutjes commented Feb 6, 2017

JanPetr left a comment

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

JanPetr Mar 29, 2017

rayrutjes commented Mar 29, 2017

maxiloc Mar 29, 2017

rayrutjes Mar 29, 2017

maxiloc Mar 29, 2017

rayrutjes Mar 29, 2017

maxiloc Mar 29, 2017

rayrutjes Mar 29, 2017

rayrutjes commented Mar 29, 2017

JanPetr commented Mar 29, 2017

rayrutjes commented Mar 29, 2017


		$this->assertCacheFileIsValid($this->failingHostsCacheFile);

		if (null === $ttl) {

Keep state between processes #195

Keep state between processes #195

Conversation

rayrutjes commented Dec 10, 2016 • edited Loading

maxiloc commented Dec 12, 2016

maxiloc commented Dec 12, 2016

rayrutjes commented Dec 12, 2016

rayrutjes commented Dec 12, 2016

rayrutjes commented Dec 12, 2016 • edited Loading

rayrutjes commented Jan 5, 2017 • edited Loading

maxiloc commented Feb 6, 2017

JanPetr commented Feb 6, 2017

rayrutjes commented Feb 6, 2017

JanPetr commented Feb 6, 2017

rayrutjes commented Feb 6, 2017

JanPetr left a 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

Choose a reason for hiding this comment

rayrutjes commented Mar 29, 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

rayrutjes commented Mar 29, 2017

JanPetr commented Mar 29, 2017

rayrutjes commented Mar 29, 2017

rayrutjes commented Dec 10, 2016 •

edited

Loading

rayrutjes commented Dec 12, 2016 •

edited

Loading

rayrutjes commented Jan 5, 2017 •

edited

Loading