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

[Aline Manera]

On 02/25/2014 03:36 AM, Royce Lv wrote:
> Reviewed-by: Royce Lv<lvroyce at 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 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())
>>           ]
>