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.
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.
Created attachment 13182 [details] [review] 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.
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.
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 ?
(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.
(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 ?
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 ?
(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.
(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 !
Created attachment 13278 [details] [review] 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.
QA comment: * code looks good * passes koha-qa.pl * no visible change if the setup is not done (as expected) passed QA
Created attachment 13315 [details] [review] 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>
This patch has been pushed to master.
Patch pushed to branch 3.10.x
To my relief, this works beautifully on Master.