[Bug 9005] New: Changing sysprefs requires restart under Plack
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Priority: P5 - low Change sponsored?: --- Bug ID: 9005 Assignee: jcamins@cpbibliography.com Summary: Changing sysprefs requires restart under Plack Severity: major Classification: Unclassified OS: All Reporter: jcamins@cpbibliography.com Hardware: All Status: NEW Version: master Component: Architecture, internals, and plumbing Product: Koha Because C4::Context uses an in-memory hash as a cache without ever updating it, anyone using Plack must restart the server any time they change a syspref. This is a serious nuisance. At the very least, set_preference should update the hash. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #1 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- It turns out we already update the cache. Unfortunately, it doesn't help if you are using a multi-threaded server, which is more-or-less a necessity. My new proposal is to make it possible to disable the cache in C4::Context. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #2 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Created attachment 13182 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=13182&action=edit Bug 9005: Allow user to disable syspref cache Because C4::Context uses an in-memory hash for caching sysprefs, changing a syspref under a multi-threaded persistent environment requires a server restart. This patch makes it possible disable the syspref cache. To test: 1) If you are using a multi-threaded persistent server (Starman, etc.), change a syspref and note that the effects of the syspref change may or may not be visible on any given request before applying this patch. You will need to choose a syspref with obvious effects that can be seen by simply refreshing the page. I recommend enabling or disabling additional languages in the OPAC, since you can refresh the page a dozen times and reasonably expect to see the new behavior you set only 1/n of the time. 2) Apply patch. 3) Add "C4::Context->disable_syspref_cache();" to your koha.psgi file 4) Repeat step 1, noting that you never see the stale behavior. 5) Run test at t/db_dependent/Context.t. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #3 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- When editing your psgi file, you will have to be very careful to make sure there are no typos. C4::Context has an autoload which means that C4::Context->X will work for all X. Also, remember to restart Plack after editing your psgi file if you don't have the server set up to do that automatically. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.poulain@biblibre.com --- Comment #4 from Paul Poulain <paul.poulain@biblibre.com> --- This patch can solve a bugguy behaviour at the price of loosing a very interesting cache feature. That can be a short-term solution only, because caching datas that are updated rarely is important What could be the long-term one ? Is there a proper way to tell Plack that he must restart all his childs ? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #5 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- (In reply to comment #4)
This patch can solve a bugguy behaviour at the price of loosing a very interesting cache feature.
No, it doesn't. It solves a major bug with no side-effects for systems that aren't affected by the bug. It is 100% opt-in.
That can be a short-term solution only, because caching datas that are updated rarely is important
If you are not changing sysprefs, don't disable the cache. I test dozens of combinations of sysprefs every day. I have a script that restarts Plack, but it's inconvenient, and an extra step that shouldn't be necessary.
What could be the long-term one ? Is there a proper way to tell Plack that he must restart all his childs ?
This seems like a bad idea from a security standpoint. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #6 from Paul Poulain <paul.poulain@biblibre.com> --- (In reply to comment #5)
(In reply to comment #4)
This patch can solve a bugguy behaviour at the price of loosing a very interesting cache feature.
No, it doesn't. It solves a major bug with no side-effects for systems that aren't affected by the bug. It is 100% opt-in. I feel that libraries that are changing sysprefs *sometimes* will complain. And we must find a generic way to solve the "rare change of datas", or we won't be able to add more caching. For example, we could store in a global/"static" variable the issuingrules, to avoid reading them on each checkin. But if they are changed, how the librarian administrator will be able to propagate it on Plack ?
investigating the documentation, I find : https://github.com/miyagawa/Starman that says : Starman is a PSGI perl web server that has unique features such as: Signals Supports "HUP" for graceful worker restarts, and "TTIN"/"TTOU" to dynamically increase or decrease the number of worker processes, as well as "QUIT" to gracefully shutdown the worker processes. wouldn't a page/link in the admin section that "reload" the server at user request be useful ?
What could be the long-term one ? Is there a proper way to tell Plack that he must restart all his childs ?
This seems like a bad idea from a security standpoint. Why is it a bad idea ?
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #7 from Paul Poulain <paul.poulain@biblibre.com> --- http://advent.plackperl.org/2009/12/day-4-reloading-applications.html says =========================== Reloading sucks? Shotgun! Reloading a module or application on a persistent perl process could cause problems, like some module package variables are redefined or overwritten and then stuck in a bad state. Plack now has Shotgun loader, which is inspired by Rack's shotgun and solves the reloading problem by loading the app on every request in a forked child environment. Using Shotgun loader is easy:
plackup -L Shotgun myapp.psgi
This will delay load the compilation of your application until the runtime, and when a request comes, it forks off a new child process to compile your app and returns the PSGI response over the pipe. You can also preload the modules that are not likely to be updated in the parent process to reduce the time needed to compile your application. For instance, if your application uses Moose and DBIx::Class, then you can say:
plackup -MMoose -MDBIx::Class -L Shotgun myapp.psgi
would speed up the time required to compile your application in the runtime. ======================== another possibility ? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #8 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- (In reply to comment #6)
(In reply to comment #5)
(In reply to comment #4)
This patch can solve a bugguy behaviour at the price of loosing a very interesting cache feature.
No, it doesn't. It solves a major bug with no side-effects for systems that aren't affected by the bug. It is 100% opt-in. I feel that libraries that are changing sysprefs *sometimes* will complain. And we must find a generic way to solve the "rare change of datas", or we won't be able to add more caching. For example, we could store in a global/"static" variable the issuingrules, to avoid reading them on each checkin. But if they are changed, how the librarian administrator will be able to propagate it on Plack ?
Sure, a generic solution is great. The solution is to use proper caching, not the mess we have in C4::Context. I favor fastmmap. However, I am not fixing now. What I am doing is trying to make my six months less frustrating than they would be if I had to restart Plack manually every couple of seconds. Global/static variables have no place in a persistent environment, in my opinion.
investigating the documentation, I find : https://github.com/miyagawa/Starman that says :
Starman is a PSGI perl web server that has unique features such as: Signals Supports "HUP" for graceful worker restarts, and "TTIN"/"TTOU" to dynamically increase or decrease the number of worker processes, as well as "QUIT" to gracefully shutdown the worker processes.
wouldn't a page/link in the admin section that "reload" the server at user request be useful ?
It might be. However, that would mean designing Koha to send SIGHUP to a web server which might or might not require setuid. Yikes! I can see if the Shotgun loader offers a workaround. However, I think the *option* to disable the cache is still needed. The patch makes no change to the current behavior for those Plack users who don't want to add "C4::Context->disable_syspref_cache();" to their psgi file. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #9 from Paul Poulain <paul.poulain@biblibre.com> --- (In reply to comment #8)
Sure, a generic solution is great. The solution is to use proper caching, not the mess we have in C4::Context. I favor fastmmap. However, I am not fixing now. What I am doing is trying to make my six months less frustrating than they would be if I had to restart Plack manually every couple of seconds.
sounds fair. And we already have Koha/Cache/Fastmmap, so, let's make it work under Plack and go that way ! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #13182|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 13278 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=13278&action=edit Bug 9005: Allow user to disable syspref cache Because C4::Context uses an in-memory hash for caching sysprefs, changing a syspref under a multi-threaded persistent environment requires a server restart. This patch makes it possible disable the syspref cache. To test: 1) If you are using a multi-threaded persistent server (Starman, etc.), change a syspref and note that the effects of the syspref change may or may not be visible on any given request before applying this patch. You will need to choose a syspref with obvious effects that can be seen by simply refreshing the page. I recommend enabling or disabling additional languages in the OPAC, since you can refresh the page a dozen times and reasonably expect to see the new behavior you set only 1/n of the time. 2) Apply patch. 3) Add "C4::Context->disable_syspref_cache();" to your koha.psgi file 4) Repeat step 1, noting that you never see the stale behavior. 5) Run test at t/db_dependent/Context.t. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> All tests passed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Version|master |3.10 QA Contact| |paul.poulain@biblibre.com --- Comment #11 from Paul Poulain <paul.poulain@biblibre.com> --- QA comment: * code looks good * passes koha-qa.pl * no visible change if the setup is not done (as expected) passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #13278|0 |1 is obsolete| | --- Comment #12 from Paul Poulain <paul.poulain@biblibre.com> --- Created attachment 13315 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=13315&action=edit Bug 9005: Allow user to disable syspref cache Because C4::Context uses an in-memory hash for caching sysprefs, changing a syspref under a multi-threaded persistent environment requires a server restart. This patch makes it possible disable the syspref cache. To test: 1) If you are using a multi-threaded persistent server (Starman, etc.), change a syspref and note that the effects of the syspref change may or may not be visible on any given request before applying this patch. You will need to choose a syspref with obvious effects that can be seen by simply refreshing the page. I recommend enabling or disabling additional languages in the OPAC, since you can refresh the page a dozen times and reasonably expect to see the new behavior you set only 1/n of the time. 2) Apply patch. 3) Add "C4::Context->disable_syspref_cache();" to your koha.psgi file 4) Repeat step 1, noting that you never see the stale behavior. 5) Run test at t/db_dependent/Context.t. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> All tests passed. Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |jcamins@cpbibliography.com --- Comment #13 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This patch has been pushed to master. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 --- Comment #14 from Paul Poulain <paul.poulain@biblibre.com> --- Patch pushed to branch 3.10.x -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9005 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED --- Comment #15 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- To my relief, this works beautifully on Master. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org