Bug 9967 - Authorized values are not Plack-compatible
Summary: Authorized values are not Plack-compatible
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Robin Sheat
QA Contact:
URL:
Keywords:
Depends on: 12041
Blocks: 12173
  Show dependency treegraph
 
Reported: 2013-04-01 22:50 UTC by Jared Camins-Esakov
Modified: 2015-06-04 23:33 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9967 - expire cached GetAuthorisedValues (1.54 KB, patch)
2014-04-03 05:33 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9967 - Cache authorised value requests properly (3.98 KB, patch)
2014-04-09 03:58 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9967 - Cache authorised value requests properly (3.48 KB, patch)
2014-04-22 05:18 UTC, Robin Sheat
Details | Diff | Splinter Review
[signed-off] Bug 9967 - Cache authorised value requests properly (3.54 KB, patch)
2014-05-21 22:43 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 9967 - include $branch_limit in the cache key (1.43 KB, patch)
2014-06-02 22:45 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 9967 - Cache authorised value requests properly (3.54 KB, patch)
2014-09-18 19:12 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 9967 - Cache authorised value requests properly (3.59 KB, patch)
2014-10-09 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9967 - include $branch_limit in the cache key (1.55 KB, patch)
2014-10-09 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2013-04-01 22:50:35 UTC
If you add or change an authorized value linked to an item field, you will not be able to use your changed authorized value under Plack without resetting the Plack server. Possibly relevant warnings:
Subroutine AuthorizedValuesForCategory redefined at /home/jcamins/kohaclone/admin/authorised_values.pl line 31.
Subroutine default_form redefined at /home/jcamins/kohaclone/admin/authorised_values.pl line 238.
Comment 1 Chris Cormack 2013-12-27 06:37:22 UTC
Jared, do you mean you can't use the value on modify item/add item screen?
Comment 2 Jared Camins-Esakov 2013-12-27 13:46:06 UTC
I tested authorized values on the item editing screen. I just did a bit more testing, and it looks like the problem might actually be with caching on the item editing screen and not the administration screen as I had thought.
Comment 3 Robin Sheat 2014-04-03 02:09:46 UTC
Attempting to reproduce:
* Plack setup with 10 workers for the staff client.
* Go to add an item to a biblio (to populate caches.)
* Added a new value in LOC, "TOASTER"
* Selected New -> Item again on the biblio.
* "Toaster" showed up in shelving location.
* Went to "Items" tab, selected an item to edit.
* "Toaster" did _not_ appear in the shelving location.
* After some other unidentifiable messing around, it did appear.

Definitely seems to be a bug.
Comment 4 Robin Sheat 2014-04-03 02:24:03 UTC
Ah, I did manage to see the issue when adding a new item too. It may depend what worker you end up hitting.

This is good, as otherwise it made no sense at all.
Comment 5 Robin Sheat 2014-04-03 03:12:10 UTC
Ah yes, there is a local cache for GetMarcStructure. It is also supposed to be memoised, but I don't see any result of that in memcache, which is strange to me.

I think the way to fix this is to:
* add a convenient ClearFrameworkCache function and call that from places like the framework editor, the authorised values editor, and so on,
* remove the memoisation of the function (which I've found tends to be slower than the local cache anyway, when it works), and
* add a cache timeout (5 minutes?) so that it won't screw up long-term if we fail to clear the cache somewhere, but still long enough that it'll likely come out of the woodwork as a bug. This'll also let us tide things over when under load.
Comment 6 Robin Sheat 2014-04-03 03:20:19 UTC
Actually, I may have spoken too soon there. It doesn't seem to actually be storing authorised values in there. So while that caching should be fixed, this particular instance might be coming from somewhere else.
Comment 7 Robin Sheat 2014-04-03 05:04:55 UTC
Oh, GetAuthorisedValues is memoised using a method that just sticks it in a hash. That'll be it.

So, there is the Memoize::Expire package that we can plug in and give authorised values a low timeout. Probably something like 10 seconds so that it'll cover that request and not much more. We can also force a clear when we set something, so that the current process will see the update immediately.
Comment 8 Robin Sheat 2014-04-03 05:33:09 UTC Comment hidden (obsolete)
Comment 9 Robin Sheat 2014-04-03 05:40:06 UTC
According to the docs, this patch _should_ work, but it's not. I've verified that it's not actually expiring the cache. However I don't know why, so if someone can have a quick look and see what I've missed, that'd be handy. If no one else can work it out, I'll add a simple hand-rolled cache to it instead.
Comment 10 Jacek Ablewicz 2014-04-03 09:09:36 UTC
(In reply to Robin Sheat from comment #9)

1/ In AddAuthorisedValue() there is a call to flush_cache('AddAuthorisedValue') instead of flush_cache('GetAuthorisedValue') - probably irrelevant.

2/ There are some Memoize and Memoize::Expire bugs reported in CPAN, perl monks etc. which are still unresolved since last available version (1.03) was published, e.g.: http://www.perlmonks.org/index.pl?node_id=1049426 .

3/ I'm wondering if memoizing GetAuthorisedValue is such a good idea in the persistent environment.. Same thing for any subroutines which return references of any kind - unless we are 100% sure than the result data structures are always used in strictly read-only mode. Looks like it's not a case for GetAuthorisedValue(): it returns reference to array of hashrefs, and it's result is often passed directly to C4::Templates::param() in various scripts. C4::Templates::output() will then traverse this result and utf8::encode() any scalar values in the data structures. Subsequent GetAuthorisedValue() calls will return the same, memoized arrayref but with contents which got possibly altered by previous request. Not pretty :).
Comment 11 Robin Sheat 2014-04-03 22:49:22 UTC
(In reply to Jacek Ablewicz from comment #10)
> 1/ In AddAuthorisedValue() there is a call to
> flush_cache('AddAuthorisedValue') instead of
> flush_cache('GetAuthorisedValue') - probably irrelevant.

Doh! But not the source of this issue.
 
> 2/ There are some Memoize and Memoize::Expire bugs reported in CPAN, perl
> monks etc. which are still unresolved since last available version (1.03)
> was published, e.g.: http://www.perlmonks.org/index.pl?node_id=1049426 .

Yeah, that's a bit fishy.

> 3/ I'm wondering if memoizing GetAuthorisedValue is such a good idea in the
> persistent environment.. Same thing for any subroutines which return
> references of any kind - unless we are 100% sure than the result data
> structures are always used in strictly read-only mode. Looks like it's not a
> case for GetAuthorisedValue(): it returns reference to array of hashrefs,

Well, it's something that gets called a lot so we need some optimisation. Really what should happen is that you ask for all the authorised values in a category, and then use the result of that for the rest of your scope. However I don't plan on refactoring that right now :)

What I'll do instead is make some simple caching functions, including the ability to a deep copy to avoid the need for a structure to never be modified. This is something that'll come up more and more with plack. I'd still expect it to be faster than a bazillion database requests, and it'll be safe enough with persistence (and can be extended to use memcache in the future should that be considered worthwhile.)

Thanks.
Comment 12 Robin Sheat 2014-04-03 22:53:12 UTC
> What I'll do instead is make some simple caching functions, including the
> ability to a deep copy to avoid the need for a structure to never be

...Or allow Koha::Cache to do this sort of thing.
Comment 13 Robin Sheat 2014-04-07 23:34:55 UTC
I've spun off the improvement of Koha::Cache into its own bug (bug 12041), as it was getting bigger.
Comment 14 Robin Sheat 2014-04-09 03:58:01 UTC Comment hidden (obsolete)
Comment 15 Robin Sheat 2014-04-22 05:18:13 UTC Comment hidden (obsolete)
Comment 16 Galen Charlton 2014-05-06 17:11:08 UTC
Resetting severity to normal - Plack support is still experimental.
Comment 17 Brendan Gallagher 2014-05-21 22:43:22 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-06-02 14:08:49 UTC
Robin, don't you think the $branch_limit variable should be part of the cache key?
Comment 19 Robin Sheat 2014-06-02 22:32:33 UTC
(In reply to Jonathan Druart from comment #18)
> Robin, don't you think the $branch_limit variable should be part of the
> cache key?

Yes, you're totally right. I'll make a followup.
Comment 20 Robin Sheat 2014-06-02 22:36:19 UTC
BTW you've probably noticed already, but when I say:

This patch depends on the caching changes of 9967.

I actually mean:

This patch depends on the caching changes of 12041.
Comment 21 Robin Sheat 2014-06-02 22:45:17 UTC Comment hidden (obsolete)
Comment 22 Brendan Gallagher 2014-09-18 19:12:53 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-10-09 13:48:42 UTC
Created attachment 32086 [details] [review]
Bug 9967 - Cache authorised value requests properly

Authorised values are now cached using the proper Koha::Cache mechanism,
rather than a simple internal cache. Memoize has been removed because it
didn't really work like we needed it to.

Test plan:
 * running a persistent environment:
 * load the edit item screen
 * refresh several times to ensure any process-based caches are filled
 * add a new LOC authorised value as fast as possible (prepare it on
   another tab.)
 * refresh the edit item page
 * note that the new LOC value may or may not be showing in the
   "shelving location"
 * if more than 5 seconds have passed since saving it, it must now show
   up.
 * refresh a few times to ensure that it's showing on all processes.

Note:
 * This patch depends on the caching changes of 9967.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 24 Jonathan Druart 2014-10-09 13:48:49 UTC
Created attachment 32087 [details] [review]
Bug 9967 - include $branch_limit in the cache key

Make the caching more correct. Also removes a warn that got left behind.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 25 Jonathan Druart 2014-10-09 13:49:52 UTC
Brendan signed the patches off.
I confirm the bug and that the patches fix it.
No regression found.
Marked as Passed QA.
Comment 26 Tomás Cohen Arazi 2014-10-11 15:05:52 UTC
Patches pushed to master.

Thanks Robin!