All caching plugins need to cope with caching servers being down

Just generalising this a bit

Notice: Memcache::flush(): Server 127.0.0.1 (tcp 11211) failed with: Connection refused (61) in /Users/danp/git/integration/cache/stores/memcache/lib.php on line 306

Thanks for the report Dan,

This is most certainly a bug.
The cache system has a couple of methods to make sure that the plugin type is usable and that any given configured instance is in a state ready to be used.
I imagine there are two things to be looked at by this issue:

1. That we are using those methods before calling plugin static methods, and before using an initialised instance.
2. That the mongodb store, as well as the other stores of similar nature, and testing the connection as part of their initialisation and replying to inquiries about their state accordingly.

I've marked this triaged and raised its priority to major.
Feel free to let me know if you think I've got that wrong.

Many thanks
Sam

Hi Sam,
this morning, while updating my VM, I got the same error:

[Sat Nov 17 10:28:46 2012] [error] [client 192.168.0.100] PHP Notice:  MemcachePool::flush(): Server localhost (tcp 11211, udp 0) failed with: Connection refused (111) in /var/www/moodle-master-git-remote/cache/stores/memcache/lib.php on line 306, referer: http://hostname/moodle-master/login/index.php
[Sat Nov 17 10:28:56 2012] [error] [client 192.168.0.100] PHP Notice:  MemcachePool::flush(): Server localhost (tcp 11211, udp 0) failed with: Connection refused (111) in /var/www/moodle-master-git-remote/cache/stores/memcache/lib.php on line 306, referer: http://hostname/moodle-master/admin/index.php
[Sat Nov 17 10:29:04 2012] [error] [client 192.168.0.100] PHP Notice:  MemcachePool::flush(): Server localhost (tcp 11211, udp 0) failed with: Connection refused (111) in /var/www/moodle-master-git-remote/cache/stores/memcache/lib.php on line 306, referer: http://hostname/moodle-master/admin/index.php
[Sat Nov 17 10:29:20 2012] [error] [client 192.168.0.100] PHP Notice:  MemcachePool::flush(): Server localhost (tcp 11211, udp 0) failed with: Connection refused (111) in /var/www/moodle-master-git-remote/cache/stores/memcache/lib.php on line 306, referer: http://hostname/moodle-master/admin/index.php?confirmupgrade=1&confirmrelease=1

The issue is that purge() for memcache store doesn't perform a check about the instance being ready to connect and that check should be performed using Memcache::connect since Memcache::addServer doesn't connect until required. I'll try to give you a patch proposal, maybe looking also at mongodb code too.

Hi Sam,
here it is, tested with a memcache cache store being memcached stopped: https://github.com/scara/moodle/compare/master...wip_m24_MDL-36322_Errors_when_purging_memcaches_mongodb_stores_during_updates.

HTH,
Matteo

Hi Sam,
when the new abstract class (MDL-36768) will land into the main stream (moodle.git) I'll update my proposal by assigning me the issue and then ask you for a peer review .

HTH,
Matteo

Hi Sam,
here is my proposal rebased to 2.4rc1.

Hi Sam
this is just a ping at the end of the year as well as a test to see if notifications now work (see MDLSITE-2066).

Again sorry about this sitting around for so long!

Thanks Matteo,

Things looked good. The memcache stores did have an error but that was easily fixed.
I've created a couple of branches based upon your branches and am putting this up for integration review now.

Many thanks
Sam

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Hi Sam,
just pulled from master, going to http://hostname/oath/to/cache/testperformance.php, I'm still receiving (debugging DEVELOPER):

Notice: MemcachePool::set(): Server localhost (tcp 11211, udp 0) failed with: Connection refused (111) in /path/to/moodle-master-git-remote/cache/stores/memcache/lib.php on line 226

I'll look at the code and eventually open a new issue.

Edit: opppsss... it's not been integrated yet! Sorry All for my silly noise... Shame on me

Hi Matteo, can you try now?

PS: Joking, It's still not integrated, LOL (sorry, couldn't resist). Thanks for your collaboration!

Touché

Oki, after a bit of chatting about this with Sam, there are only some leftovers to clarify, before allowing this to land:

1) If we are moving the connection stuff (when present) from initialise() to constructor... then we need to clarify that in the API and make it clear which are the responsibilities for each method. For example, the mongo store right now explicitly is saying "but does not connect to it" and that's 100% wrong if we apply the patch. So please, try to clarify it in as many places as possible.

2) There are some clone() operations in create_store_from_config(), and it causes me a bit of stress about all those resources (connections...) being clonable always. Sam did some tests with mongo and seemed to work... but... not sure how to behave about this point (unit test somehow, move it to some other place... not sure).

Waiting for some feedback... otherwise I think I understand it and looks ok.

Ciao

Hi Eloy,

Thanks for the two points:

Re 1: I'll go through code and review/update the docs no probs and clarify the difference. I'll also make mention of it somewhere in the docs.

Re 2: Unfortunately at present we don't have any unit tests for stores that are not default.
I'm pretty sure I could knock up some sort of unit tests if we want to go down that path. I've been thinking it over for a while.
We already have the performance test script that requires a store instance.
With a little core code change we could ensure that when running unit tests we use those test instances if available then we could write an abstract test class that could be extended by stores to perform any tests we require.

So what to do right now?

Presently it is a bit of a conundrum.
It is expected that a stores initialisation will always succeed.

If a store isn't going to deal with a connection/property being cloned then there are options (the first two that come to mind):
a) In the constructor open the connection to test that is works and then close it. Within the initialisation open the connection and store it.
b) Create a means to open a connection from outside for the cache store, and then have that means hold onto any established connections and hand them out if requested again in the future. In the constructor request a connection to check it works but don't hold onto it, and then in the init request it again.

So there is a work around to this approach which is good, but we'll need to document it well so that people understand. It's going to mean writing a cache store is a little more advanced but I think it is doable.

Perhaps the other idea, the alternative to this approach would be to write a means to allow store initialisation to notify of success/failure.
Upon failure we'd simply not use it, and should all fail we'd have to either use the default store, or use the dummy store (nothing cached really).
I've not fully gone through the code but I think this wouldn't be too difficult to implement.
The advantage to this would be that we would not need to change store logic so drastically. They'd just need to return true on success and false on failure.

So options:

1. Integrate this now. Create a new issue to create unit tests and test for this case.
2. Create an issue for unit tests, reopen this and make it dependent on the unit tests being integrated first.
3. Scrap this change and allow initialisation to fail and deal with it in cache API. Create a new issue to create unit tests and test for this case.

What are your thoughts Eloy, I'm 50/50 between #1 and #3 presently.

Many thanks
Sam

I'm a bit worried about the proposal of starting the connections just to verify they work and close them immediately, to be reopened again by init. Sounds like a slowdown in my mind, doing the same twice for each request.

Perhaps one alternative solution maybe to provide to the API one store_clone() (note surely the name is not correct), that, by default will perform a simple clone() but, for stores not supporting cloning, will enable the developer to re-initialize anything?

Not sure if it has sense or no, just asking... ciao

Using a method to control cloning sounds like a good option to me as well.

Worth noting is that the magic method __clone is available as well although that is called immediately after the object has been cloned.
It would allow an object to do anything it wanted immediately after the clone which I imagine would cover all cases.
The only situation that it would not cover would be if an error/exception was generated when trying to clone the store. I imagine that would be highly unlikely however (and surely unstable design).

Either we live with __clone or we implement our own method that would be responsible for running the clone routine.
It could be done either before or after this gets in, I'm happy either way

Perhaps we can do that there, so the API only changes once, and everybody gets directions properly? That would mean delay this 1-week, but I think that it's better than doing it in 2 steps.

So my +1 goes to reopen and implement the missing phpdocs & clone, if that's a solution really.

Ciao

Agreed to give this one more run, so reopening, thanks!

Thanks Eloy, I'm putting this back up for integration review now with two additional commits:

1. Clarifies the phpdocs for the __construct and initialise methods.
2. Implements a cache_store::create_clone method to handle the production of a clone. This way any stores with an issue when cloning can override that method to address issues if required.

Many thanks
Sam

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Reading through this and looking at the code it looks good to me. I'm gonna get Eloy to +1 it.

Integrated to master and 24. Thanks everyone!

// Taking Andrew's testing issue.

Tested this issue for 2.4 and Master. It works great. The notices and errors are not longer exist.

However, with my limitation of understanding caching entirely, I'm not 100% percent sure I did the test correctly. Second opinion or tester for this issue would be great.

Thanks.

Rosie

Hi Sam,

Just noting an error when trying to delete MongoDB (with bad config) store instance.

Exception - Failed to connect to: 127.0.9.1:27017: Authentication failed on database 'admin' with username 'user': test fails

More information about this error
Debug info:
Error code: generalexceptionmessage
Stack trace:

    line 466 of /cache/stores/mongodb/lib.php: MongoConnectionException thrown
    line 466 of /cache/stores/mongodb/lib.php: call to Mongo->__construct()
    line 315 of /cache/locallib.php: call to cachestore_mongodb->instance_deleted()
    line 126 of /cache/admin.php: call to cache_config_writer->delete_store_instance()

Looks like we've missed a spot.. I think this should be failed.

Failing this issue due to the MongoDB deletion error.

Sorry.

A fix has been integrated for this now. Thanks for picking up the issue Rosie.
I've moved this back to ready for testing now.

I have restested this on 24 and master and the test instructions complete correctly.

Thanks Sam

Passing

Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

Regards, Damyon