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

[Rodrigo Trujillo]

Reviewed-by: Rodrigo Trujillo <rodrigo.trujillo at linux.vnet.ibm.com>
Tested-by: Rodrigo Trujillo <rodrigo.trujillo at linux.vnet.ibm.com>

Tested in Fedora 20

On 02/25/2014 12:54 AM, Aline Manera wrote:
> From: Aline Manera <alinefm at 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 at 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())
>           ]