Bug 10503 - incorrect initialization parameters passed to Memoize::Memcached
Summary: incorrect initialization parameters passed to Memoize::Memcached
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Galen Charlton
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-24 18:47 UTC by Galen Charlton
Modified: 2019-10-14 19:56 UTC (History)
6 users (show)

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


Attachments
bug 10503: fix various issues with use of Memoize::Memcached (6.87 KB, patch)
2013-06-25 15:29 UTC, Galen Charlton
Details | Diff | Splinter Review
bug 10503: fix various issues with use of Memoize::Memcached (6.85 KB, patch)
2013-06-25 16:06 UTC, Galen Charlton
Details | Diff | Splinter Review
[SIGNED-OFF] bug 10503: fix various issues with use of Memoize::Memcached (6.90 KB, patch)
2013-07-17 23:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2013-06-24 18:47:04 UTC
Currently, Memoize::Memcached is initialized like this:

  eval {
      if (C4::Context->ismemcached) {
          require Memoize::Memcached;
          import Memoize::Memcached qw(memoize_memcached);
  
          memoize_memcached( 'GetMarcStructure',
                              memcached => C4::Context->memcached);
      }
  };

C4::Context->memcached returns a Cache::Memcached object.  However, that's not what Memoize::Memcached expects:

  This function also accepts a memcached option, which expects a hashref. 
  This is de-referenced and passed directly into an internal function which
  sets up the memcached configuration for that function. This contents of this
  hashref are mostly options passed to Cache::Memcached, with a few exceptions.

However, passing a Cache::Memcached happened to work, by coincidence, as  Memoize::Memcached was calling UNIVERSAL::isa (in a deprecated fashion [0]) to check to see whether the passed argument is a HASH.

Nowadays, though, since UNIVERSAL->import is deprecated, the version of Memoize::Memcached packaged for Debian Wheezy now uses ref() to check whether the argument is a hash [1].  That means that the coincidence no longer works, and consequently MARC frameworks don't get cached via memcached in Debian Wheezy, leading to a performance degradation.

The solution is to pass Memoize::Memcached what it needs -- a hashref with the Cache::Memcached arguments -- and not a Cache::Memcached object.

[0] https://rt.cpan.org/Public/Bug/Display.html?id=68170
[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=614868
Comment 1 Galen Charlton 2013-06-25 15:29:31 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-06-25 15:44:41 UTC
Note to testers:

After running through the test plan, this is what I got when I ran memcdump:

$ memcdump --servers 127.0.0.1:11211
KOHAGMCismemcached
KOHAGMC970239eca4adb0bb2bb8f4dc761a3023
KOHAGMCmemoize-getTranslatedLanguages-scalar-intranet
KOHAGMCkohaconf
KOHAGMCmemoize-_get_columns-scalar-borrowers
KOHAGMCmemoize-GetMarcStructure-scalar-1

The key names will vary based on what you set MEMCACHED_NAMESPACE to.
Comment 3 Galen Charlton 2013-06-25 16:06:49 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2013-07-16 02:49:08 UTC
I can see GetMarcStructure and getTranslatedLanguages, but not _get_columns
Comment 5 Galen Charlton 2013-07-17 18:46:05 UTC
(In reply to Srdjan Jankovic from comment #4)
> I can see GetMarcStructure and getTranslatedLanguages, but not _get_columns

I tried again and it works for me:

$ memcdump --servers localhost:11211
KOHAismemcached
KOHAmemoize-getTranslatedLanguages-scalar-intranet
KOHAmemoize-getTranslatedLanguages-list-intranet
KOHAkohaconf
KOHAmemoize-_get_columns-scalar-borrowers

Could you try again, and if it doesn't work, provide more details about your test environment?
Comment 6 Srdjan Jankovic 2013-07-17 23:41:23 UTC
Created attachment 19735 [details] [review]
[SIGNED-OFF] bug 10503: fix various issues with use of Memoize::Memcached

Memoize::Memcached expects to be passed a hashref of connection
parameters, which it in turn uses to initialize a Cache::Memcached
object.  However, commit 52afe06ddd started passing a Cache::Memcached
object directly, not the required hashref.  This happened to work
by coincidence, but no longer works with recent versions of
Memoize::Memcached packaged for Debian.

This patch introduces a new C4::Context function, memcached_params(), that
returns a hashref of connection parameters suitable for passing to
the Cache::Memcached constructor.  It also introduces a UNITCHECK block
to C4::Context so that memcached_params() can be used during intialization
of the module.

This patch also corrects SQLHelper so that it no longer tries to
use the deprecated (and removed) memcached_servers setting in koha-conf.xml;
it now gets the memcached connection parameters from C4::Context->memcached_params().

To test:

[1] This patch is most easily tested on Debian Wheezy, as version
    0.03-2 of the libmemoize-memcached-perl package includes the fix
    for Debian bug 614868 that causes Koha's use of Memoize::Memcached
    to break.  If you don't want to go so far as to install/upgrade to
    Wheezy, you can apply the following Debian patch to your copy
    of Memoize::Memcached to get the effect:

    http://patch-tracker.debian.org/patch/series/view/libmemoize-memcached-perl/0.03-2/universal.patch

[2] Set up your Koha Apache configuration to point to a memcached server.
[3] Restart memcached to start with a clear cache.
[4] Do the following steps:
    - Create a new bib record.  Note that it is sufficient to
      just open the editor; you don't need to save the bib.
    - Search for a patron record.
[5] Use the memcdump command-line to get a list of keys in your memcached cache.
    You should *not* see keys that contain the strings GetMarcStructure,
    getTranslatedLanguages, or _get_columns.
[6] Apply the patch, then go through step 4 again.
[7] Use memcdump again.  This time, you *should* see the keys
    mentioned in step 5.
[8] Set the SessionStorage system preference to memcached.  After logging in
    again, verify that your staff interface and OPAC sessions work without
    requiring that you log in for each and every page load.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Comment 7 Srdjan Jankovic 2013-07-17 23:42:55 UTC
Sorry, it is there. I'm going to see my optometrist today, that should help.
Comment 8 Jonathan Druart 2013-09-04 15:11:29 UTC
Looks good to me, the patch works as expected on Debian Wheezy.

> This patch also corrects SQLHelper so that it no longer tries to
> use the deprecated (and removed) memcached_servers setting in koha-conf.xml;

Why do you say that?
it is still in used by Koha/Template/Plugin/Cache.pm and appears in debian/templates/koha-conf-site.xml.in.

Should we remove these occurrences too?
Comment 9 Mason James 2013-09-13 09:20:24 UTC
(In reply to Jonathan Druart from comment #8)
> Looks good to me, the patch works as expected on Debian Wheezy.
> 
> > This patch also corrects SQLHelper so that it no longer tries to
> > use the deprecated (and removed) memcached_servers setting in koha-conf.xml;
> 
> Why do you say that?
> it is still in used by Koha/Template/Plugin/Cache.pm and appears in
> debian/templates/koha-conf-site.xml.in.
> 
> Should we remove these occurrences too?

hmm, lets switch to 'in discussion'...
Comment 10 Jared Camins-Esakov 2013-09-22 18:10:45 UTC
(In reply to Jonathan Druart from comment #8)
> Looks good to me, the patch works as expected on Debian Wheezy.
> 
> > This patch also corrects SQLHelper so that it no longer tries to
> > use the deprecated (and removed) memcached_servers setting in koha-conf.xml;
> 
> Why do you say that?
> it is still in used by Koha/Template/Plugin/Cache.pm and appears in
> debian/templates/koha-conf-site.xml.in.
> 
> Should we remove these occurrences too?

That would be a good idea, yes.
Comment 11 Jonathan Druart 2013-09-23 12:06:48 UTC
I think the 2 occurrences remaining should be replace.
Marked as Failed QA.
Comment 12 Marc Véron 2016-10-04 16:13:34 UTC
Still valid?
Comment 13 Jonathan Druart 2018-06-29 12:35:47 UTC
No longer valid.