Bug 16441 - C4::Letters::getletter is not plack safe
Summary: C4::Letters::getletter is not plack safe
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: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 9452
Blocks: 7172 16624
  Show dependency treegraph
 
Reported: 2016-05-04 18:43 UTC by Jonathan Druart
Modified: 2017-12-07 22:15 UTC (History)
5 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 16441: Do not use a package variable to cache C4::Letters::getletter (1.81 KB, patch)
2016-05-04 18:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16441: Do not use a package variable to cache C4::Letters::getletter (1.86 KB, patch)
2016-05-23 02:52 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16441: Fix Letters.t (1.83 KB, patch)
2016-06-07 10:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16441: Fix Letters.t (1.93 KB, patch)
2016-06-07 13:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16441: Do not use a package variable to cache C4::Letters::getletter (2.05 KB, patch)
2016-06-10 09:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16441: Fix Letters.t (1.93 KB, patch)
2016-06-10 09:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-05-04 18:43:30 UTC
C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine.
I have not found any direct issues caused by that but it is safer to remove it.
It won't be a big deal to hit the DBMS to get a valid letter when needed.
Comment 1 Jonathan Druart 2016-05-04 18:44:48 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2016-05-23 02:52:36 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-06-01 12:21:55 UTC
(In reply to Jonathan Druart from comment #0)
> It won't be a big deal to hit the DBMS to get a valid letter when needed.

Hm. Not sure. getletter is called by GetPreparedLetter, SendCirculationAlert, etc. In loops ?

-# FIXME: using our here means that a Plack server will need to be
-#        restarted fairly regularly when working with this routine.
-#        A better option would be to use Koha::Cache and use a cache
-#        that actually works in a persistent environment, but as a
-#        short-term fix, our will work.

Why not use this opportunity to add caching?
Comment 4 Jonathan Druart 2016-06-01 13:24:40 UTC
(In reply to Marcel de Rooy from comment #3)
> (In reply to Jonathan Druart from comment #0)
> > It won't be a big deal to hit the DBMS to get a valid letter when needed.
> 
> Hm. Not sure. getletter is called by GetPreparedLetter,
> SendCirculationAlert, etc. In loops ?
> 
> -# FIXME: using our here means that a Plack server will need to be
> -#        restarted fairly regularly when working with this routine.
> -#        A better option would be to use Koha::Cache and use a cache
> -#        that actually works in a persistent environment, but as a
> -#        short-term fix, our will work.
> 
> Why not use this opportunity to add caching?

Because I have not found anywhere it's called many times. As I wrote previously, I am not sure it'd be interesting to cache it, but I may be wrong.
If we decide to cache it, we need to make sure the values in cache are deleted when letters are updated/deleted.
Anyway it will be a much bigger patch and more an enhancement than a bugfix.
Comment 5 Marcel de Rooy 2016-06-03 16:56:56 UTC
(In reply to Jonathan Druart from comment #4)
> Because I have not found anywhere it's called many times. As I wrote
> previously, I am not sure it'd be interesting to cache it, but I may be
> wrong.
> If we decide to cache it, we need to make sure the values in cache are
> deleted when letters are updated/deleted.
> Anyway it will be a much bigger patch and more an enhancement than a bugfix.

Yes, it would be more work. But this patch affects performance.
Note that I am not sure if this is actually a bugfix.
The comment about regularly restarting plack is probably outdated. If we let each worker process only handle 50 requests, you do not need to restart plack very often.
Since a new worker starts with an empty 'cache', you will not long see the old letter contents anymore.
Comment 6 Marcel de Rooy 2016-06-04 07:24:46 UTC
(In reply to Marcel de Rooy from comment #5)
> The comment about regularly restarting plack is probably outdated. If we let
> each worker process only handle 50 requests, you do not need to restart
> plack very often.

--max-requests
Number of the requests to process per one worker process. Defaults to 1000.

git grep max-requests:
debian/scripts/koha-plack:    STARMANOPTS="-M FindBin --max-requests 50 --workers 2 \
misc/plack/plackup.sh:# --max-requests 50 decreased from 1000 to keep memory usage sane
misc/plack/plackup.sh:opt="$opt --server Starman -M FindBin --max-requests 50 --workers 4"
Comment 7 Jonathan Druart 2016-06-07 10:50:16 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-06-07 10:51:42 UTC
Working on tests for bug 16624, I have found that the tests were wrong, but we were not aware of that.
Another point to remove this cache :)
Comment 9 Marcel de Rooy 2016-06-07 13:32:22 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2016-06-07 14:09:21 UTC
Planning to come back here soon :)
Comment 11 Marcel de Rooy 2016-06-10 09:14:30 UTC
Created attachment 52217 [details] [review]
Bug 16441: Do not use a package variable to cache C4::Letters::getletter

C4::Letters::getletter use a package variable (%letter) to cache letter
returned by the subroutine.
I have not found any direct issues caused by that but it is safer to
remove it.
It won't be a big deal to hit the DBMS to get a valid letter when
needed.

No test plan here, just confirm that the changes make sense.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Confirm that performance loss is just a millisecond or so per
subsequent call of getletter.
Comment 12 Marcel de Rooy 2016-06-10 09:14:35 UTC
Created attachment 52218 [details] [review]
Bug 16441: Fix Letters.t

These tests were wrong, but the package variable used to cache the
letters hid the problem.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Kyle M Hall 2016-06-10 16:42:20 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 14 Frédéric Demians 2016-06-15 07:41:20 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 15 Julian Maurice 2016-06-16 08:39:13 UTC
Patches pushed to 3.22.x, will be in 3.22.8