Re: [Kimchi-devel] [PATCH] issue #325: Use RamSession instead of FileSession

Reviewed-by: Royce Lv<lvroyce(a)linux.vnet.ibm.com> According to http://www.stderr.org/doc/cherrypy-doc/html/howto/node17.html, it is said that RAM session is not applicable to multi-process environment. If this will not be problem in our current environment, I guess we can add it to commit message, so that if we encounter problem related to this, we can track to this spot.

On 2014年02月25日 11:54, Aline Manera wrote: > From: Aline Manera <alinefm(a)br.ibm.com&gt; > > There is a readers/writes problem in cherry.sessions.FileSession > implementation which makes it non thread safe. > > From https://groups.google.com/forum/#!topic/cherrypy-users/biitlom41T8 > "When the session tool is turned on it hooks the lib.session.save > function onto 'before_finalize' so that the session data is always > re-saved at the end of each request. For RamSession this is fine > because save just overwrites a dictionary value. FileSession uses > pickle.dump to write the session data to file and uses pickle.load to > load it. If two requests overlap in just the wrong way then it's > possible that pickle.load gets called halfway through the pickle.dump > which throws an EOFError. More concurrent requests will increase the > chance of this happening." > > On Host tab, more concurrent requests happen, specially while creating a > debug report: /host/stats and /tasks/<id>. Because that the problem was > identified there. > > To solve this issue, there are 2 options: > 1) overwrite cherrypy.session.FileSession._load to fix the > readers/writers and make it thread safe; > or > 2) Use RamSession instead of FileSession > > As using RamSession makes the application faster because it will not > need to > read/write file for each request, this is the best solution in this > case. > > This patch also uses acquire_lock() and release_lock() for each > read/write session data occurrence. > > Signed-off-by: Aline Manera <alinefm(a)br.ibm.com&gt; > --- > src/kimchi/auth.py | 22 +++++++++++++--------- > src/kimchi/config.py.in | 4 ---- > src/kimchi/server.py | 4 +--- > 3 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py > index 94e7eab..fda4e1f 100644 > --- a/src/kimchi/auth.py > +++ b/src/kimchi/auth.py > @@ -131,13 +131,13 @@ def check_auth_session(): > A user is considered authenticated if we have an established > session open > for the user. > """ > - try: > - if cherrypy.session[USER_ID]: > - debug("Session authenticated for user %s" % > - cherrypy.session[USER_ID]) > - return True > - except KeyError: > - pass > + cherrypy.session.acquire_lock() > + session = cherrypy.session.get(USER_ID, None) > + cherrypy.session.release_lock() > + if session is not None: > + debug("Session authenticated for user %s" % session) > + return True > + > debug("Session not found") > return False > > @@ -188,11 +188,15 @@ def logout(): > cherrypy.lib.sessions.expire() > > > + > def has_permission(admin_methods): > + cherrypy.session.acquire_lock() > + session = cherrypy.session.get(USER_ID, None) > + cherrypy.session.release_lock() > + > return not admin_methods or \ > cherrypy.request.method not in admin_methods or \ > - (cherrypy.request.method in admin_methods and > - cherrypy.session[USER_SUDO]) > + (cherrypy.request.method in admin_methods and session) > > > def kimchiauth(admin_methods=None): > diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in > index 32d61c6..92d5b91 100644 > --- a/src/kimchi/config.py.in > +++ b/src/kimchi/config.py.in > @@ -36,10 +36,6 @@ from kimchi.xmlutils import xpath_get_text > DEFAULT_LOG_LEVEL = "debug" > > > -def get_session_path(): > - return os.path.join(paths.state_dir, 'sessions') > - > - > def get_object_store(): > return os.path.join(paths.state_dir, 'objectstore') > > diff --git a/src/kimchi/server.py b/src/kimchi/server.py > index 6dd0404..ef8e701 100644 > --- a/src/kimchi/server.py > +++ b/src/kimchi/server.py > @@ -73,8 +73,7 @@ class Server(object): > 'tools.sessions.name': 'kimchi', > 'tools.sessions.httponly': True, > 'tools.sessions.locking': 'explicit', > - 'tools.sessions.storage_type': 'file', > - 'tools.sessions.storage_path': config.get_session_path(), > + 'tools.sessions.storage_type': 'ram', > 'tools.kimchiauth.on': False}, > '/css': { > 'tools.staticdir.on': True, > @@ -136,7 +135,6 @@ class Server(object): > os.path.dirname(os.path.abspath(options.error_log)), > os.path.dirname(os.path.abspath(config.get_object_store())), > os.path.abspath(config.get_screenshot_path()), > - os.path.abspath(config.get_session_path()), > os.path.abspath(config.get_debugreports_path()), > os.path.abspath(config.get_distros_store()) > ]