[Bug 16441] New: C4::Letters::getletter is not plack safe
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Bug ID: 16441 Summary: C4::Letters::getletter is not plack safe Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org Depends on: 9452 Blocks: 7172 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7172 [Bug 7172] Omnibus for Plack variable scoping problems https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9452 [Bug 9452] C4::Letters does not function under Plack -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 51218 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51218&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51218|0 |1 is obsolete| | --- Comment #2 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 51703 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51703&action=edit [SIGNED-OFF] 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52120 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52120&action=edit Bug 16441: Fix Letters.t These tests were wrong, but the package variable used to cache the letters hid the problem. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16624 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16624 [Bug 16624] Times are formatted incorrectly in slips ( AM PM ) due to double processing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52120|0 |1 is obsolete| | --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 52130 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52130&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Planning to come back here soon :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 52217 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52217&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52130|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 52218 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52218&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51703|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |kyle@bywatersolutions.com --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11, thanks Jonathan! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr --- Comment #14 from Frédéric Demians <frederic@tamil.fr> --- Pushed in 16.05. Will be in 16.05.01. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16441 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |julian.maurice@biblibre.com --- Comment #15 from Julian Maurice <julian.maurice@biblibre.com> --- Patches pushed to 3.22.x, will be in 3.22.8 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org