[PATCH] issue #325: Use RamSession instead of FileSession
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
From: Aline Manera <alinefm@br.ibm.com> There is a readers/writes problem in cherry.sessions.FileSession implementation which makes it non thread safe. 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@br.ibm.com> --- 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()) ] -- 1.7.10.4
Reviewed-by: Royce Lv<lvroyce@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@br.ibm.com>
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@br.ibm.com> --- 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()) ]
On 02/25/2014 03:36 AM, Royce Lv wrote:
Reviewed-by: Royce Lv<lvroyce@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.
Sure. I will add this link to commit message.
On 2014年02月25日 11:54, Aline Manera wrote:
From: Aline Manera <alinefm@br.ibm.com>
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@br.ibm.com> --- 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=sessio... 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 <alinefm@br.ibm.com>
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@br.ibm.com> --- 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(???)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 02/25/2014 10:42 AM, Sheldon wrote:
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=sessio... 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.
I am not sure the different between those 2 sessions. I will try to figure out
On 02/25/2014 11:54 AM, Aline Manera wrote:
From: Aline Manera<alinefm@br.ibm.com>
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<alinefm@br.ibm.com> --- 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(???)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 2/25/2014 8:42 AM, Sheldon wrote:
Not sure RamSession is a good choice. What are your concerns with RamSession?
What's the different between MemcachedSession and RamSession.
Memcached is a distributed memory cache: http://en.wikipedia.org/wiki/Memcached which seems like overkill given that we do not presently have a clustering solution. At this point we are using the session to cache information about the user: groups that they are in whether or not they have sudo others? All of those are fairly simple to reconstruct in the case that the session gets lost or destroyed. i.e. RamSession seems appropriate for the current session usage pattern. -- Adam King <rak@linux.vnet.ibm.com> IBM C&SI
2014/2/25 23:22, Adam King:
On 2/25/2014 8:42 AM, Sheldon wrote:
Not sure RamSession is a good choice. What are your concerns with RamSession?
What's the different between MemcachedSession and RamSession.
Memcached is a distributed memory cache: http://en.wikipedia.org/wiki/Memcached which seems like overkill given that we do not presently have a clustering solution.
At this point we are using the session to cache information about the user: groups that they are in whether or not they have sudo others?
The user session will totally lost When Kimchi server get reboots and this is a big surprise to the user.
All of those are fairly simple to reconstruct in the case that the session gets lost or destroyed. i.e. RamSession seems appropriate for the current session usage pattern.
Even for RamSession, we need to check that no racing condition exists when two requests access the big session dictionary at the same time. I searched a bit, no clear answer yet.
On 02/25/2014 12:47 PM, Shu Ming wrote:
2014/2/25 23:22, Adam King:
On 2/25/2014 8:42 AM, Sheldon wrote:
Not sure RamSession is a good choice. What are your concerns with RamSession?
What's the different between MemcachedSession and RamSession.
Memcached is a distributed memory cache: http://en.wikipedia.org/wiki/Memcached which seems like overkill given that we do not presently have a clustering solution.
At this point we are using the session to cache information about the user: groups that they are in whether or not they have sudo others?
The user session will totally lost When Kimchi server get reboots and this is a big surprise to the user.
I don't think it is a problem.
All of those are fairly simple to reconstruct in the case that the session gets lost or destroyed. i.e. RamSession seems appropriate for the current session usage pattern.
Even for RamSession, we need to check that no racing condition exists when two requests access the big session dictionary at the same time. I searched a bit, no clear answer yet.
It is already done by this patch: acquire_lock() and release_lock() while accessing the session data
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
Reviewed-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> Tested-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> Tested in Fedora 20 On 02/25/2014 12:54 AM, Aline Manera wrote:
From: Aline Manera <alinefm@br.ibm.com>
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@br.ibm.com> --- 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()) ]
I am not sure it is right to use ram session for Kimchi server. Using ram session will cause current session data lost after the Kimchi server is rebooted. In web server design, it is very common to allow the web server to restart without the user in the fronend to know. So the session data should be persistent across the reboot process for the user. Also, the web server can be restarted intentionally for maintenance or randomly caused by bugs. 2014/2/25 11:54, Aline Manera :
From: Aline Manera <alinefm@br.ibm.com>
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@br.ibm.com> --- 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()) ]
Am 25-02-2014 12:21, schrieb Shu Ming:
In web server design, it is very common to allow the web server to restart without the user in the fronend to know. Is it?
Even though we will disconnect the user when the Kimchi server reboots, we will have more benefits by applying this patch: fix the bug mentioned by Aline and make some requests faster. The other proposal to solve the problem (overriding cherrypy.session.FileSession._load) doesn't look very nice, so this is still the best option.
On 02/25/2014 12:21 PM, Shu Ming wrote:
I am not sure it is right to use ram session for Kimchi server. Using ram session will cause current session data lost after the Kimchi server is rebooted. In web server design, it is very common to allow the web server to restart without the user in the fronend to know. So the session data should be persistent across the reboot process for the user. Also, the web server can be restarted intentionally for maintenance or randomly caused by bugs. I don't think this is a big issue.
Honestly, first time I tried Kimchi, I found quite strange that my login session was kept alive after a server reboot. After thinking a little about it, I concluded that this was default behavior for lots of web services we use everyday. However, even being the default out there, it doesn't mean I think it is always the correct decision. I personally think that keeping the session alive forever (as it is usual with a famous web mail, for instance) is a bad design choice and a security flaw as others can easily access my account in a shared computer if I don't log out from the session. On the other hand, I agree that I don't want to log in again every time the web server is rebooted. In an ideal world the user would never know when the web server is rebooted. Anyway, in the case we are specifically talking about, I don't think the most important thing is the session data. As Adam pointed out in another e-mail, the session data we are storing currently is very easy to retrieve again. The only important state the session is storing is whether a user is logged in or not. In this case, I think it is preferable to ask the user to log in again after a server reboot than to let the server fail because of a cherrypy bug. I don't think that overwriting the cherrypy function is a viable solution as well. This is something that needs to be fixed in cherrypy itself and we can use it in the future when it is integrated into the distros. For the time being, RamSession seems to be a good compromise. I would like also to propose an extension to the current proposal, even though I don't think this extension is mandatory to be implemented before getting this fix upstream. When the web server is gracefully rebooted (through a SIGTERM or SIGINT signal) we can save the current session in a file before exiting the server and, when the server starts again, we can check if a previous session is available and load it. That way we would not lost the logged in session between web server reboots. This does not prevent the case in which we loose the session if a bug is hit in the web server and it crashes. But in this case we have a bug anyway and we will need to provide a fix for the bug, so I don't think it is a case we need to worry about. Best regards, Leonardo Garcia
2014/2/25 11:54, Aline Manera :
From: Aline Manera <alinefm@br.ibm.com>
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@br.ibm.com> --- 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()) ]
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (8)
-
Adam King -
Aline Manera -
Crístian Viana -
Leonardo Augusto Guimarães Garcia -
Rodrigo Trujillo -
Royce Lv -
Sheldon -
Shu Ming