quetz-mod_python-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Gallacher <...@jgassociates.ca>
Subject Re: get_session(), req.session, req.form and MODPYTHON-38
Date Tue, 14 Mar 2006 00:33:42 GMT
Graham Dumpleton wrote:
> Grisha wrote ..
> 
>>On Mon, 13 Mar 2006, Graham Dumpleton wrote:
>>
>>
>>>Thus I want a documented convention that if a handler is going to use
>>>util.FieldStorage, that it should before doing so, first check whether
>>>an existing instance resides as req.form and use that instead.
>>
>>I'm not sure if this is a good example - req.form is something specific
>>to 
>>the publisher. Rather than perhaps documenting it as you suggest, 
>>util.FieldStorage can take it upon itself to create a req.form so that
>>subsequent attempts to instantiate it just return req.form. (This is just
>>an example, I'm not 100% sure that I having FS do this makes sense - seems
>>like a good idea).
>>
>>
>>>Similarly, if a handler is going to create a Session object, that it
>>>look for an existing instance as req.session and again use that instead.
>>
>>OR, the Session module would know to look for a req.session, in which case
>>the handlers wouldn't need to worry about it.
>>
>>(One thing to watch out for would be that mutliple concurrent sessions
>>in the same request is a possibility)

The problem is that you are still depending on a naming convention which 
requires 2 things from users: they read the docs and they adopt the 
convention. Both are losing propostions, IMHO. Heck, *I* don't read the 
docs. :)

The idea of something like req.get_session() is to give users an obvious 
way to grab a session object without the deadlock concerns. How many 
times have we seen this kind of problem-code on the mailing list?

def index(req):
     sess = Session.Session(req)
     do_stuff(req)
     ...

def do_stuff(req):
     sess = Session.Session(req)
     ... do other stuff.

Having the session constructor check for the existence of req.session is 
of no use here. We need a way to make sure only *one* session instance 
is created per request. (Bonus marks for making it work with internal 
redirect).

> 
> Hmmm, having a look at the code, at some point the check for req.session
> got added and I didn't realise or forgot that it had been done.
> 
>         # does this code use session?
>         session = None
>         if "session" in code.co_names:
>             if hasattr(req, 'session'):
>                 session = req.session
>             else:
>                 session = Session.Session(req)
> 
> It didn't get added for form though, which means that accessing form
> arguments from within a PSP page will mean only those in the query
> string of the URL will be available as the content of the request has
> already been consumed.

It didn't get added for form handling because I was "fixing" the session 
code. ;)  To me the session problem is the more serious of the two. Mess 
up your form code and you have to rewrite your code. This is 
inconvenient. Mess up your session code and you deadlock your server. 
This is very bad.

> Looks like a audit of both:
> 
>   https://issues.apache.org/jira/browse/MODPYTHON-38
>   https://issues.apache.org/jira/browse/MODPYTHON-59
> 
> need to be done to work out what has and hasn't been done related
> to this so we know where we are up to.
> 
> Looks a bit like when the req.get_session() changes got rolled back that
> it got introduced at that point:
> 
>   http://svn.apache.org/viewcvs.cgi//httpd/mod_python/trunk/lib/python/mod_python/psp.py?rev=226320&view=diff&r1=226320&r2=226319&p1=/httpd/mod_python/trunk/lib/python/mod_python/psp.py&p2=/httpd/mod_python/trunk/lib/python/mod_python/psp.py
> 
> Before the req.get_session() change it didn't exist:
> 
>   http://svn.apache.org/viewcvs.cgi//httpd/mod_python/trunk/lib/python/mod_python/psp.py?rev=191745&view=diff&r1=191745&r2=191744&p1=/httpd/mod_python/trunk/lib/python/mod_python/psp.py&p2=/httpd/mod_python/trunk/lib/python/mod_python/psp.py
> 
> I can't remember if this was a conscious decision to check for req.session
> based on suggestions or otherwise.
> 
> Graham

Although the psp.py code in question was put in when I was messing 
around with get_session, it is not an artifact of get_session being 
incompletely rolled back. Ignoring the whole get_session stuff for a 
moment, the diff would look like this:

Index: psp.py
===================================================================
--- psp.py	(revision 164956)
+++ psp.py	(revision 226320)
@@ -190,7 +190,10 @@
          # does this code use session?
          session = None
          if "session" in code.co_names:
-            session = Session.Session(req)
+            if hasattr(req, 'session'):
+                session = req.session
+            else:
+                session = Session.Session(req)


This new code helps to avoid the deadlock problem if the user had the 
foresight to stuff their session object into req. The biggest problem 
with this change is not it's validity, but the fact that it was not 
documented.

Anyway, the get_session code in requestobject.c was half-baked and 
should not have been checked into trunk. Furthermore, when it was 
decided to defer this issue to 3.3, I should have expunged it completely 
rather than leaving it as a stub. I guess at the time I was drunk with 
my new-found committer power. Mea Culpa. (We have some other code 
scattered about in the source which is commented out. Reading it you 
just scratch your head ask yourself WTF? We really shouldn't allow this 
kind of cruft to accumulate.)

I still think there is an argument to be made for a get_session()-like 
functionality to more tightly control access to the session object.

Oh, and just to make it clear to everyone, all I did was assign 
MODPYTHON-59 to myself so it doesn't get lost in the shuffle. It 
happens to be one of our oldest issues, and since I created I thought I 
better take responsibility for it.  I don't have any master plan to 
foist this code on anyone. :)

Jim




Mime
View raw message