Menu

#53 Report original exception for session unpickle error

closed
WebKit (45)
5
2003-02-18
2003-01-31
Oleg Noga
No

The purpose of this patch is to provide the original
session unpickle exception instead of KeyError
exception.
---
Since we have no corrupted sessions on the disk, the
Session KeyError is still appears.
KeyError appears if SessionFileStore can't unpickle
session while loading it from disk.
Session can't be unpickled only if it has objects, with
have buggy implemented unpickling feature.
Developer, who works under webkit, would like to fix
unpickling for his session.
So, match better for developer to see the original
exception instead of KeyError.
---
This patch was made on Webware0.8b1.

Discussion

  • Oleg Noga

    Oleg Noga - 2003-01-31

    The patch

     
  • Oleg Noga

    Oleg Noga - 2003-01-31
    • assigned_to: nobody --> stuartd
     
  • Geoff Talvola

    Geoff Talvola - 2003-02-04

    Logged In: YES
    user_id=88162

    Wouldn't this part of the patch be easier if we just said
    "raise"? raise with no arguments re-raises the latest
    exception. There's no need to rebuild it from sys.exc_info().

    + e = sys.exc_info()
    + raise e[0], e[1], e[2] # raise the exception again, so
    developers can look on it

     
  • Geoff Talvola

    Geoff Talvola - 2003-02-04

    Logged In: YES
    user_id=88162

    I'm not sure it's a good idea to re-raise the unpickling
    exception here. It may be better to _print_ the unpickling
    exception and possibly email it to the administrator, but
    still raise KeyError so that the application can treat this
    as a missing session and possibly recover more gracefully.

     
  • Oleg Noga

    Oleg Noga - 2003-02-04

    Logged In: YES
    user_id=551440

    Here is KeyError traceback:

    Traceback (most recent call last):
    File ".\WebKit\Application.py", line 368, in dispatchRequest
    elif self.isSessionIdProblematic(request):
    File ".\WebKit\Application.py", line 466, in
    isSessionIdProblematic
    if (time()-request.session().lastAccessTime()) >=
    request.session().timeout():
    File ".\WebKit\HTTPRequest.py", line 225, in session
    return self._transaction.session()
    File ".\WebKit\Transaction.py", line 67, in session
    self._session =
    self._application.createSessionForTransaction(self)
    File ".\WebKit\Application.py", line 889, in
    createSessionForTransaction
    session = self.session(sessId)
    File ".\WebKit\Application.py", line 725, in session
    return self._sessions[sessionId]
    File ".\WebKit\SessionDynamicStore.py", line 72, in
    __getitem__
    self.MovetoMemory(key)
    File ".\WebKit\SessionDynamicStore.py", line 135, in
    MovetoMemory
    self._memoryStore[key] = self._fileStore[key]
    File ".\WebKit\SessionFileStore.py", line 55, in __getitem__
    raise KeyError, key
    KeyError: 20030204232946-
    b46a22352890568b1a899c361d816933

    Seems that KeyError have thrown out of awake-respond-sleep
    cycle (before it), so servlet can’t perform any gracefull
    recoveries. Maybe application must recover the same way as
    when session can’t be pickled? To remove unpickable
    session file, to create new session, and not to throw any
    exception at all?
    Any way, it usefull to see original exception report – in
    console/log, or in http exception report.
    ---
    I have raised exception like “raise e[0], e[1], e[2]” just to
    preserve original traceback. If original traceback has no
    helpful information, we can simple “raise <original exception
    instance>”
    The simplest patch is to remove try/except at all :)

     
  • Stuart Donaldson

    Logged In: YES
    user_id=326269

    The original traceback is preserved if you just do "raise"
    without any arguments. It just re-raises the existing
    exception along with existing traceback information.

    From a consistency point of view, raising a KeyError for
    problems retrieving a session seems to be pretty common.
    While I think this specific case is not caught, there are
    other cases where a session stores __getitem__ throws a
    KeyError that does get caught.

    From a users point of view, shouldn't this just appear as a
    missing session file?

    From a developers view, you would want to see what the
    unpickling error was.

    If these two assumptions are correct, then perhaps a
    solution like this:

    - catch KeyError in Application.createSessionForTransaction,
    and treat it like there was no session.

    - have the decoding exception raise a key error as it does
    now, but also print out a the stack trace for a developer,
    possibly calling handleException() to log the exceptino as well.

     
  • Oleg Noga

    Oleg Noga - 2003-02-05

    Logged In: YES
    user_id=551440

    So I've produced patches:
    for Application: Application.diff
    for SessionfileStore.diff

    in SessionFileStore.__getitem__:
    * close session file finally (seems like bug fix)
    in case of session unpicling error:
    1. remove session file
    2. call Application.handleException()
    3. raise KeyError

    in Application.createSessionForTransaction:
    1. catch KeyError
    2. create new session if it was not loaded from store

     
  • Oleg Noga

    Oleg Noga - 2003-02-05

    in replace of SessionFileStore2.diff

     
  • Oleg Noga

    Oleg Noga - 2003-02-05

    in addition to SessionFileStore.diff

     
  • Stuart Donaldson

    new patch works within isSessionIdProblematic()

     
  • Stuart Donaldson

    Logged In: YES
    user_id=326269

    Check out the revised Application2.diff I am uploading.

    This places the catch of the KeyError in
    isSessionIdProblematic() which is called specifically to see
    if a session ID is valid when processing dispatchRequest().
    If it fails (typically because the sesion has expired) it
    marks the session as expired so the servlet can check and
    handle the situation.

    It has the advantage of cleaning up _SID_ cookies and fields
    on the request and going through handleInvalidSession which
    honors the setting 'IgnoreInvalidSession'

    Please advise if this solves your problem. Note it should
    be used in addition to the SessionFileStore.diff patch which
    raises the KeyError.

     
  • Oleg Noga

    Oleg Noga - 2003-02-14

    Logged In: YES
    user_id=551440

    Aplication2.diff in couple with SessionFileStore.diff seems
    works fine.
    Request reports like session expired if session had unpickle
    problems. And we haveoriginal traceback in console.
    Thanks.

     
  • Stuart Donaldson

    • status: open --> closed
     
  • Stuart Donaldson

    Logged In: YES
    user_id=326269

    Committed fix to CVS.

     

Log in to post a comment.