Bug 21955

Summary: Cache::Memory should not be used as L2 cache
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: dcook, fridolin.somers, jesse, julian.maurice, martin.renvoize, mtj, nick
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19523
Change sponsored?: --- Patch complexity: ---
Who signed the patch off: Text to go in the release notes:
Cache::Memory fails to work correctly under a plack environment as the cache cannot be shared between processes.
Version(s) released in:
Attachments: Bug 21955: Remove Cache::Memory as fallback for L2 cache
Bug 21955: Remove Cache::Memory as fallback for L2 cache
Bug 21955: Remove Cache::Memory as fallback for L2 cache

Description Jonathan Druart 2018-12-05 18:33:30 UTC
If Plack is running with several workers we must not use Cache::Memory as L2 cache.

If a value is set from a worker, it will not be available from other workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job, so we should not expect performance regressions by removing it.
Comment 1 Jonathan Druart 2018-12-05 19:44:16 UTC
Created attachment 82890 [details] [review]
Bug 21955: Remove Cache::Memory as fallback for L2 cache

If Plack is running with several workers we must not use Cache::Memory
as L2 cache.

If a value is set from a worker, it will not be available from other
workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job,
so we should not expect performance regressions by removing it.

See also the email sent to koha-devel for more info
http://lists.koha-community.org/pipermail/koha-devel/2018-December/045004.html
Comment 2 Jonathan Druart 2018-12-05 19:45:03 UTC
Maybe the severity can be adjusted later if not considered as critical.
Comment 3 Martin Renvoize 2018-12-10 11:10:14 UTC
Created attachment 83007 [details] [review]
Bug 21955: Remove Cache::Memory as fallback for L2 cache

If Plack is running with several workers we must not use Cache::Memory
as L2 cache.

If a value is set from a worker, it will not be available from other
workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job,
so we should not expect performance regressions by removing it.

See also the email sent to koha-devel for more info
http://lists.koha-community.org/pipermail/koha-devel/2018-December/045004.html

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2018-12-10 11:10:37 UTC
I'm fully behind this, signing off.
Comment 5 Julian Maurice 2018-12-10 15:57:48 UTC
I cannot reproduce the issue you mentioned on the ML.

about.pl says

> Effective caching method: Cache::Memory

I have 2 starman workers but reloading the page always show the right value for the syspref.
What am I missing ?
Comment 6 Jonathan Druart 2018-12-10 19:00:05 UTC
(In reply to Julian Maurice from comment #5)
> I cannot reproduce the issue you mentioned on the ML.
> 
> about.pl says
> 
> > Effective caching method: Cache::Memory
> 
> I have 2 starman workers but reloading the page always show the right value
> for the syspref.
> What am I missing ?

Can you confirm you are displaying the pref's value using the Koha.Preference method, i.e. not looking at the pref admin view, which fetches the value from the DB directly?
Also make sure starman's parameter --max-requests is not set to 1 :)
Comment 7 Julian Maurice 2018-12-11 08:27:00 UTC
Ah sorry, I saw 2 starman processes and interpreted that as 2 workers, but it was the master and 1 worker.
If I increase the number of workers, I can reproduce the problem indeed.
Comment 8 Julian Maurice 2018-12-11 08:27:22 UTC
Created attachment 83034 [details] [review]
Bug 21955: Remove Cache::Memory as fallback for L2 cache

If Plack is running with several workers we must not use Cache::Memory
as L2 cache.

If a value is set from a worker, it will not be available from other
workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job,
so we should not expect performance regressions by removing it.

See also the email sent to koha-devel for more info
http://lists.koha-community.org/pipermail/koha-devel/2018-December/045004.html

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 9 Nick Clemens 2018-12-11 20:56:47 UTC
Awesome work all!

Pushed to master for 19.05
Comment 10 Martin Renvoize 2018-12-12 10:34:25 UTC
Pushed to 18.11.x for 18.11.01
Comment 11 Jesse Maseto 2018-12-18 19:24:06 UTC
Pushed to 18.05.x for 18.05.07
Comment 12 Fridolin SOMERS 2018-12-19 13:30:43 UTC
Pushed to 17.11.x for 17.11.13