Bug 9005 - Changing sysprefs requires restart under Plack
Summary: Changing sysprefs requires restart under Plack
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low major (vote)
Assignee: Jared Camins-Esakov
QA Contact: Paul Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-01 18:03 UTC by Jared Camins-Esakov
Modified: 2013-12-05 20:01 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9005: Allow user to disable syspref cache (5.49 KB, patch)
2012-11-01 19:06 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 9005: Allow user to disable syspref cache (5.60 KB, patch)
2012-11-07 12:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9005: Allow user to disable syspref cache (5.61 KB, patch)
2012-11-08 17:40 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-11-01 18:03:33 UTC
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.
Comment 1 Jared Camins-Esakov 2012-11-01 18:07:14 UTC
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.
Comment 2 Jared Camins-Esakov 2012-11-01 19:06:45 UTC Comment hidden (obsolete)
Comment 3 Jared Camins-Esakov 2012-11-01 19:14:14 UTC
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.
Comment 4 Paul Poulain 2012-11-01 22:16:37 UTC
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 ?
Comment 5 Jared Camins-Esakov 2012-11-01 22:23:32 UTC
(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.
Comment 6 Paul Poulain 2012-11-01 22:39:14 UTC
(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 ?
Comment 7 Paul Poulain 2012-11-01 22:42:06 UTC
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 ?
Comment 8 Jared Camins-Esakov 2012-11-01 22:52:46 UTC
(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.
Comment 9 Paul Poulain 2012-11-01 23:04:15 UTC
(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 !
Comment 10 Kyle M Hall 2012-11-07 12:06:52 UTC Comment hidden (obsolete)
Comment 11 Paul Poulain 2012-11-08 17:40:29 UTC
QA comment:
 * code looks good
 * passes koha-qa.pl
 * no visible change if the setup is not done (as expected)

passed QA
Comment 12 Paul Poulain 2012-11-08 17:40:56 UTC
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>
Comment 13 Jared Camins-Esakov 2012-11-08 17:46:53 UTC
This patch has been pushed to master.
Comment 14 Paul Poulain 2012-11-12 13:21:51 UTC
Patch pushed to branch 3.10.x
Comment 15 Jared Camins-Esakov 2012-12-03 13:06:04 UTC
To my relief, this works beautifully on Master.