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()) > ]

Not sure RamSession is a good choice. What's the different between MemcachedSession and RamSession. Have check the cherrpy doc, does not tell the details. http://docs.cherrypy.org/en/latest/refman/lib/sessions.html?highlight=ses... cherrypy.lib.sessions.init(/storage_type='ram'/, /path=None/, /path_header=None/, /name='session_id'/, /timeout=60/, /domain=None/, /secure=False/, /clean_freq=5/, /persistent=True/, /httponly=False/, /debug=False/, /**kwargs/) Initialize session object (using cookies). storage_type One of 'ram', 'file', 'postgresql', 'memcached'. This will be used to look up the corresponding class in cherrypy.lib.sessions globals. For example, 'file' will use the FileSession class.

On 02/25/2014 11:54 AM, Aline Manera wrote: > From: Aline Manera&lt;alinefm(a)br.ibm.com&gt; > > There is a readers/writes problem in cherry.sessions.FileSession > implementation which makes it non thread safe. > > >Fromhttps://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&lt;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()) > ] -- Thanks and best regards! Sheldon Feng(???)&lt;shaohef(a)linux.vnet.ibm.com&gt; IBM Linux Technology Center

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()) ]