You are viewing a plain text version of this content. The canonical link for it is here.
Posted to mod_python-dev@quetz.apache.org by Jim Gallacher <ji...@jgassociates.ca> on 2005/04/11 06:41:19 UTC
FileSession.py - improved? version
Hello All,
I've attached an alternative version of FileSession.py. A couple of
changes from the original version:
1. Uses PythonOption FileSessionDir to determine directory where session
files will be save. If this option does not exists, it defaults to
tempfile.tempdir().
2. Does file locking independent of the session locking. File locking
always occurs, while session locking is still optional. I'm finding that
session locking still causes some problems, so I've changed the default
for session locking to 0.
3. Fixed a bug in do_cleanup which was calling time(), not time.time()
and added import time statement.
4. Added do_cleanup() optimizations. Two parameters now control the
behaviour of do_cleanup(). The session file mtime can be used to
determine if the file is a deletion candidate. If it is it can either be
deleted immediately or unpickled and the true session expiration
verified. The speedup can be significant for more than 10,000 sessions.
Regards,
Jim
Re: FileSession.py - improved? version
Posted by Jim Gallacher <jg...@sympatico.ca>.
Hi Nicolas,
Nicolas Lehuen wrote:
> OK, I've checked your code in, but I have a remark : the number of
> global locks is limited in mod_python. That's why you have something
> like "mod_python: Creating 32 session mutexes based on 0 max processes
> and 250 max threads." in the error.log file.
I've been wondering about that. Thanks for confirming. I'm very aware
that the locking scheme for my FileSession is less than ideal.
> By having a look at _global_lock and _global_unlock in
> _apachemodule.c, you will see that any request for a lock will be
> brought down to one of the N-1 (here 32 - 1 = 31) pre-allocated global
> locks (the -1 stands for the lock 0 which is special, see the code).
> Hence, with more than 31 concurrent requests, you WILL have
> collisions. What I fear is that those collisions are deadlock-prone...
To say nothing of lock starvation, where some application programing
error causes a request to not return and thus the lock for that session
is never released. I wonder if this would explain the problems I've seen
with session locks turned. The file locks created should always be
released since the file manipulations are surrounded by a try - finally
clause, whereas the session lock release is depending on the method
registered by the req.register_cleanup in BaseSession.lock(). Is
anything registered with register_clean() absolutely guaranteed to run,
no matter what? I'll need to read the code.
> In that case, I wonder if it is worthy to have two different locking
> mechanisms (at the session level and a the file level) since it will
> make collisions even more probable. You solve this by disabling
> session locking by default, but session locking can truly be useful
> (for example, when you have a site which loads a frameset in which
> each frame requires a session).
I realize session locking is useful, but I turned it off by default
since it seemed to be causing too many problems. This was intended as a
temporary measure until I could figure out what was going on. I can
imagine someone grabbing FileSession.py from svn, using it as a drop-in
replacement for DbmSession on a production machine, and killing apache
as a result.
> I think we should find a way to have reentrant session locking, so
> that if a process/thread already holds a session lock, a new call to
> BaseSession.lock() does not block the process/thread. This way, we
> could simply enable session locking by default and re-use the same
> locks for file-level locking. This would enable to have file-level
> locking even if session-level locking is disabled, while keeping the
> number of locks (and lock collisions) low.
This would be ideal.
> Or, we could try to re-evaluate the need for this finite global lock
> pool. I guess Grisha implemented this for the sake of performance
> (apparently global mutexes can be expensive in some implementations of
> the APR). Why not create a distinct lock for each lock name (and thus
> exceed the 32 limit) ?
Well, it's always going to be finite limit, and some idiot like me will
always find a way to exceed that limit. :)
Regards,
Jim
> On Apr 11, 2005 6:41 AM, Jim Gallacher <ji...@jgassociates.ca> wrote:
>
>>Hello All,
>>
>>I've attached an alternative version of FileSession.py. A couple of
>>changes from the original version:
>>
>>1. Uses PythonOption FileSessionDir to determine directory where session
>>files will be save. If this option does not exists, it defaults to
>>tempfile.tempdir().
>>
>>2. Does file locking independent of the session locking. File locking
>>always occurs, while session locking is still optional. I'm finding that
>>session locking still causes some problems, so I've changed the default
>>for session locking to 0.
>>
>>3. Fixed a bug in do_cleanup which was calling time(), not time.time()
>>and added import time statement.
>>
>>4. Added do_cleanup() optimizations. Two parameters now control the
>>behaviour of do_cleanup(). The session file mtime can be used to
>>determine if the file is a deletion candidate. If it is it can either be
>>deleted immediately or unpickled and the true session expiration
>>verified. The speedup can be significant for more than 10,000 sessions.
>>
>>Regards,
>>Jim
>>
>>
>>
>
>
Re: FileSession.py - improved? version
Posted by Barry Pearce <ba...@copyrightwitness.net>.
Er....in which case...Doesnt this bring back the conversation we had on
locking and the use of lock files....supporting a million concurrent
requests via file locking is possible, and not by these mutexes -
although a little slower you will not notice them...I use this code
already...
Perhaps my conversation on locking was as much a tangent as I first
thought...
Nicolas Lehuen wrote:
> OK, I've checked your code in, but I have a remark : the number of
> global locks is limited in mod_python. That's why you have something
> like "mod_python: Creating 32 session mutexes based on 0 max processes
> and 250 max threads." in the error.log file.
>
> By having a look at _global_lock and _global_unlock in
> _apachemodule.c, you will see that any request for a lock will be
> brought down to one of the N-1 (here 32 - 1 = 31) pre-allocated global
> locks (the -1 stands for the lock 0 which is special, see the code).
> Hence, with more than 31 concurrent requests, you WILL have
> collisions. What I fear is that those collisions are deadlock-prone...
>
> In that case, I wonder if it is worthy to have two different locking
> mechanisms (at the session level and a the file level) since it will
> make collisions even more probable. You solve this by disabling
> session locking by default, but session locking can truly be useful
> (for example, when you have a site which loads a frameset in which
> each frame requires a session).
>
> I think we should find a way to have reentrant session locking, so
> that if a process/thread already holds a session lock, a new call to
> BaseSession.lock() does not block the process/thread. This way, we
> could simply enable session locking by default and re-use the same
> locks for file-level locking. This would enable to have file-level
> locking even if session-level locking is disabled, while keeping the
> number of locks (and lock collisions) low.
>
> Or, we could try to re-evaluate the need for this finite global lock
> pool. I guess Grisha implemented this for the sake of performance
> (apparently global mutexes can be expensive in some implementations of
> the APR). Why not create a distinct lock for each lock name (and thus
> exceed the 32 limit) ?
>
> Regards,
> Nicolas
>
> On Apr 11, 2005 6:41 AM, Jim Gallacher <ji...@jgassociates.ca> wrote:
>
>>Hello All,
>>
>>I've attached an alternative version of FileSession.py. A couple of
>>changes from the original version:
>>
>>1. Uses PythonOption FileSessionDir to determine directory where session
>>files will be save. If this option does not exists, it defaults to
>>tempfile.tempdir().
>>
>>2. Does file locking independent of the session locking. File locking
>>always occurs, while session locking is still optional. I'm finding that
>>session locking still causes some problems, so I've changed the default
>>for session locking to 0.
>>
>>3. Fixed a bug in do_cleanup which was calling time(), not time.time()
>>and added import time statement.
>>
>>4. Added do_cleanup() optimizations. Two parameters now control the
>>behaviour of do_cleanup(). The session file mtime can be used to
>>determine if the file is a deletion candidate. If it is it can either be
>>deleted immediately or unpickled and the true session expiration
>>verified. The speedup can be significant for more than 10,000 sessions.
>>
>>Regards,
>>Jim
>>
>>
>>
>
>
Re: FileSession.py - improved? version
Posted by "Gregory (Grisha) Trubetskoy" <gr...@apache.org>.
On Mon, 11 Apr 2005, Nicolas Lehuen wrote:
> By having a look at _global_lock and _global_unlock in _apachemodule.c,
> you will see that any request for a lock will be brought down to one of
> the N-1 (here 32 - 1 = 31) pre-allocated global locks (the -1 stands for
> the lock 0 which is special, see the code). Hence, with more than 31
> concurrent requests, you WILL have collisions.
Yes. If you search the mailing list archives from long-long ago (don't
know if they're available online), there should be a thread on the logic
behind it. The main idea is that apache only processes as many concurrent
requests as there are threads/processes available, therefore having as
many locks as there are sessions is a waste, you only need no more than
apache can process.
On top of that consider that only a fraction of requests are session
requests - images and other static files shoudn't be session based.
So what seems like a big problem at first, isn't really. You _will_ have
collisions with this scheme, but all that it does is make one session wait
for another. The probablilty of this happening is relatively low, and this
seems like a nice compromise.
Also, all this is relevant if session locks are important to you. They're
on by default, bu if you're _really_ concerned with performance you should
consider having them off.
On some OS's (namely certain popular variations of RedHat) the lock of
choice picked by APR is the SysV semaphore. There is a finite number of
them and if not released properly you will get "no space left on device"
when trying to allocate a lock. If you look at the Fedora mod_python RPM,
you'll see that they set MAX_LOCKS in mod_python.h from 32 to 4, and
that's actually not an unreasonable option IMO.
> What I fear is that those collisions are deadlock-prone...
A dead-lock is a BUG. There are no dead-locks in the current session
implementation to the best of my knowledge, we just need to make sure same
is true for FileSession :-).
Grisha
Re: FileSession.py - improved? version
Posted by David Fraser <da...@sjsoft.com>.
Nicolas Lehuen wrote:
>OK, I've checked your code in, but I have a remark : the number of
>global locks is limited in mod_python. That's why you have something
>like "mod_python: Creating 32 session mutexes based on 0 max processes
>and 250 max threads." in the error.log file.
>
>By having a look at _global_lock and _global_unlock in
>_apachemodule.c, you will see that any request for a lock will be
>brought down to one of the N-1 (here 32 - 1 = 31) pre-allocated global
>locks (the -1 stands for the lock 0 which is special, see the code).
>Hence, with more than 31 concurrent requests, you WILL have
>collisions. What I fear is that those collisions are deadlock-prone...
>
>In that case, I wonder if it is worthy to have two different locking
>mechanisms (at the session level and a the file level) since it will
>make collisions even more probable. You solve this by disabling
>session locking by default, but session locking can truly be useful
>(for example, when you have a site which loads a frameset in which
>each frame requires a session).
>
>I think we should find a way to have reentrant session locking, so
>that if a process/thread already holds a session lock, a new call to
>BaseSession.lock() does not block the process/thread. This way, we
>could simply enable session locking by default and re-use the same
>locks for file-level locking. This would enable to have file-level
>locking even if session-level locking is disabled, while keeping the
>number of locks (and lock collisions) low.
>
>Or, we could try to re-evaluate the need for this finite global lock
>pool. I guess Grisha implemented this for the sake of performance
>(apparently global mutexes can be expensive in some implementations of
>the APR). Why not create a distinct lock for each lock name (and thus
>exceed the 32 limit) ?
>
>
Even if the 32 lock limit is required, it may be possible to use the
global locks to access other locks that are not global in the same way.
For example, if you have 32 global mutexes, you can use each one to
control access to an area of shared memory that is not individually
locked, but contains multiple lock states. So the granularity of the
locking is larger, but you can still lock / unlock individual sessions.
Not sure if I've desccribed that in a way that makes any sense or would
be helpful, but anyway...
David
Re: FileSession.py - improved? version
Posted by Jim Gallacher <jg...@sympatico.ca>.
Hi Grisha,
Gregory (Grisha) Trubetskoy wrote:
>
> On Tue, 12 Apr 2005, Jim Gallacher wrote:
>
>> However, when lock=0, there will be a race condition. Two process can
>> have their own copies of FileSession data for a specific session at
>> the same time.
>
>
> OK, but that's not specific to the FileSession? It sounded like there is
> something inherent in FileSession about this.
It's not specific to FileSession - that's just the context of the
discussion. I wanted to note that there was a race condition in case
anyone was looking at the evolving FileSession code.
> This is basically the main reason why session locking is necessary, it'd
> be good add this to documentation since it's not very easy to grasp why
> session locking is necessary. But it belongs in BaseSession docs I think.
Agreed. When the FileSession code has settled down, I'll write some
documentation. I can modify BaseSession documentation section detailing
this limitatation as well, if you like. Or would it make sense to have a
separate section describing the importance of session locking, since it
is, as you say, difficult to grasp?
Regards,
Jim
Re: FileSession.py - improved? version
Posted by "Gregory (Grisha) Trubetskoy" <gr...@apache.org>.
On Tue, 12 Apr 2005, Jim Gallacher wrote:
> However, when lock=0, there will be a race condition. Two process can have
> their own copies of FileSession data for a specific session at the same time.
OK, but that's not specific to the FileSession? It sounded like there is
something inherent in FileSession about this.
This is basically the main reason why session locking is necessary, it'd
be good add this to documentation since it's not very easy to grasp why
session locking is necessary. But it belongs in BaseSession docs I think.
Grisha
Re: FileSession.py - improved? version
Posted by "Gregory (Grisha) Trubetskoy" <gr...@apache.org>.
On Tue, 12 Apr 2005, Jim Gallacher wrote:
> The only way I can see around it (the race condition, not the documentation),
> is to make session locking mandatory. Any thoughts?
I think we need to say that it is absolutely necessary, but still make it
optional to leave the user an option of implementing their own locking
mechanism.
Grisha
Re: FileSession.py - improved? version
Posted by Jim Gallacher <jg...@sympatico.ca>.
Hi Grisha,
Gregory (Grisha) Trubetskoy wrote:
>
> On Mon, 11 Apr 2005, Jim Gallacher wrote:
>
>> There will still be a race condition when session locking is off, but
>> at least the integrity of the session files is preserved. This race
>> condition should be noted in the forthcoming documentation though.
>
>
> Why not when locking is oh retain the lock for the duration of the
> request and when locking is off only retain it for the duration of the
> read/write operation?
This is exactly what has been implemented. However, as with DbmSession,
session locking can be turned off. When session locking is off, a lock
is still acquired for read/write operations to protect the file
integrity. However, when lock=0, there will be a race condition. Two
process can have their own copies of FileSession data for a specific
session at the same time. Even with file locking, one will still
overwrite the other. It's race. The data stored in the file may not be
valid, but at least it won't be corrupt. The only way to avoid it is to
have session locking turned on with lock=1.
Correct me if I'm wrong, but this same race condition exits in
DbmSession as well, when lock=0.
> A documented race condition doesn't sound like a
> good idea :-)
True, but it's better than an undocumented race condition :)
The only way I can see around it (the race condition, not the
documentation), is to make session locking mandatory. Any thoughts?
Regards,
Jim
Re: FileSession.py - improved? version
Posted by "Gregory (Grisha) Trubetskoy" <gr...@apache.org>.
On Mon, 11 Apr 2005, Jim Gallacher wrote:
> There will still be a race condition when session locking is off, but at
> least the integrity of the session files is preserved. This race condition
> should be noted in the forthcoming documentation though.
Why not when locking is oh retain the lock for the duration of the request
and when locking is off only retain it for the duration of the read/write
operation? A documented race condition doesn't sound like a good idea :-)
Grisha
Re: FileSession.py - improved? version
Posted by Jim Gallacher <jg...@sympatico.ca>.
Nicolas Lehuen wrote:
> On Apr 11, 2005 6:35 PM, Jim Gallacher <jg...@sympatico.ca> wrote:
>
>>Hi Nicolas,
>>
>>Attached is a new version of FileSession.py that uses a different
>>locking scheme. In a "well duh" kind of moment I thought why not just
>>check if the session is locked before trying to lock the file? It seems
>>to work so I've changed the default locking back on.
>
>
> The reason why I haven't done this before is because lock reentrance
> is not as simple as it seems. We need a rock solid solution here, a
> proven solution, because threading bugs are such a mess to debug that
> we can't afford to see one appear. So we'll have to check all corner
> cases, such as :
>
> 1) What if two threads / two processes enter are initializing their
> session "at the same time" ? What if there was a thread switch between
> the moment the self._locked==1 test is done and the moment the lock is
> performed ? In that case, the session would be locked twice, then
> unlocked once, which could cause problems.
>
> 2) What if self.locked==1, so we skip locking, then immediatly there's
> a thread switch and the lock is released in another thread ? In that
> case, the session would not be locked at all, with possible problems
> if a third thread tries to read while we're writing.
>
> So the reentrancy check really needs to be atomically performed with
> the lock, which is not something you can easily do if you don't have a
> bit of hardware (or virtual machine) support, somewhere. And don't
> even get me started with the problem of double-checked locking...
>
> Fortunately, those two cases are made impossible by the fact that the
> FileSession instances are not shared between threads. Each thread has
> its own copy of the FileSession, the data being shared between threads
> by loading and saving data to a file in a serial fashion. So the two
> cases above won't happen, and we can safely utter this "well, duh",
> now that we've carefully thought about it :) Consequently, I've
> checked in your code.
I've reviewed your comments and walked through the code again just to
make sure. Looks OK, since as you point out each thread gets it's own
copy of FileSession.
There will still be a race condition when session locking is off, but at
least the integrity of the session files is preserved. This race
condition should be noted in the forthcoming documentation though. As
someone commented earlier, if the user turns session locking off, they
better know what they are doing.
One thorny issue I've chosen to ignore so far is the lack of file
locking in the do_cleanup method. Lots of interesting suprises hiding
there, I'm sure.
Regards,
Jim
Re: FileSession.py - improved? version
Posted by Nicolas Lehuen <ni...@gmail.com>.
On Apr 11, 2005 6:35 PM, Jim Gallacher <jg...@sympatico.ca> wrote:
> Hi Nicolas,
>
> Attached is a new version of FileSession.py that uses a different
> locking scheme. In a "well duh" kind of moment I thought why not just
> check if the session is locked before trying to lock the file? It seems
> to work so I've changed the default locking back on.
The reason why I haven't done this before is because lock reentrance
is not as simple as it seems. We need a rock solid solution here, a
proven solution, because threading bugs are such a mess to debug that
we can't afford to see one appear. So we'll have to check all corner
cases, such as :
1) What if two threads / two processes enter are initializing their
session "at the same time" ? What if there was a thread switch between
the moment the self._locked==1 test is done and the moment the lock is
performed ? In that case, the session would be locked twice, then
unlocked once, which could cause problems.
2) What if self.locked==1, so we skip locking, then immediatly there's
a thread switch and the lock is released in another thread ? In that
case, the session would not be locked at all, with possible problems
if a third thread tries to read while we're writing.
So the reentrancy check really needs to be atomically performed with
the lock, which is not something you can easily do if you don't have a
bit of hardware (or virtual machine) support, somewhere. And don't
even get me started with the problem of double-checked locking...
Fortunately, those two cases are made impossible by the fact that the
FileSession instances are not shared between threads. Each thread has
its own copy of the FileSession, the data being shared between threads
by loading and saving data to a file in a serial fashion. So the two
cases above won't happen, and we can safely utter this "well, duh",
now that we've carefully thought about it :) Consequently, I've
checked in your code.
> Tested with and without session locking and have not seen any problems
> so far:
> ab -n 500000 http://localhost/session_test.py
>
> Regards,
> Jim
>
>
>
Re: FileSession.py - improved? version
Posted by Jim Gallacher <jg...@sympatico.ca>.
Nicolas Lehuen wrote:
> OK, I've checked your code in, but I have a remark : the number of
> global locks is limited in mod_python. That's why you have something
> like "mod_python: Creating 32 session mutexes based on 0 max processes
> and 250 max threads." in the error.log file.
>
> By having a look at _global_lock and _global_unlock in
> _apachemodule.c, you will see that any request for a lock will be
> brought down to one of the N-1 (here 32 - 1 = 31) pre-allocated global
> locks (the -1 stands for the lock 0 which is special, see the code).
> Hence, with more than 31 concurrent requests, you WILL have
> collisions. What I fear is that those collisions are deadlock-prone...
>
> In that case, I wonder if it is worthy to have two different locking
> mechanisms (at the session level and a the file level) since it will
> make collisions even more probable. You solve this by disabling
> session locking by default, but session locking can truly be useful
> (for example, when you have a site which loads a frameset in which
> each frame requires a session).
>
> I think we should find a way to have reentrant session locking, so
> that if a process/thread already holds a session lock, a new call to
> BaseSession.lock() does not block the process/thread. This way, we
> could simply enable session locking by default and re-use the same
> locks for file-level locking. This would enable to have file-level
> locking even if session-level locking is disabled, while keeping the
> number of locks (and lock collisions) low.
>
> Or, we could try to re-evaluate the need for this finite global lock
> pool. I guess Grisha implemented this for the sake of performance
> (apparently global mutexes can be expensive in some implementations of
> the APR). Why not create a distinct lock for each lock name (and thus
> exceed the 32 limit) ?
>
> Regards,
> Nicolas
>
Hi Nicolas,
Attached is a new version of FileSession.py that uses a different
locking scheme. In a "well duh" kind of moment I thought why not just
check if the session is locked before trying to lock the file? It seems
to work so I've changed the default locking back on.
class FileSession(BaseSession):
def lock_file(self):
if not self._locked:
self._req.log_error('lock file %s' % self._sid)
_apache._global_lock(self._req.server, self._sid)
self._locked = 1
def unlock_file(self):
if self._locked and not self._lock:
self._req.log_error('unlock file %s' % self._sid)
_apache._global_unlock(self._req.server, self._sid)
self._locked = 0
# pseudo code to demonstrate
def do_test(self):
self.lock_file()
try:
do_file_read_or_write_stuff()
finally:
self.unlock_file()
Tested with and without session locking and have not seen any problems
so far:
ab -n 500000 http://localhost/session_test.py
Regards,
Jim
Re: FileSession.py - improved? version
Posted by Nicolas Lehuen <ni...@gmail.com>.
OK, I've checked your code in, but I have a remark : the number of
global locks is limited in mod_python. That's why you have something
like "mod_python: Creating 32 session mutexes based on 0 max processes
and 250 max threads." in the error.log file.
By having a look at _global_lock and _global_unlock in
_apachemodule.c, you will see that any request for a lock will be
brought down to one of the N-1 (here 32 - 1 = 31) pre-allocated global
locks (the -1 stands for the lock 0 which is special, see the code).
Hence, with more than 31 concurrent requests, you WILL have
collisions. What I fear is that those collisions are deadlock-prone...
In that case, I wonder if it is worthy to have two different locking
mechanisms (at the session level and a the file level) since it will
make collisions even more probable. You solve this by disabling
session locking by default, but session locking can truly be useful
(for example, when you have a site which loads a frameset in which
each frame requires a session).
I think we should find a way to have reentrant session locking, so
that if a process/thread already holds a session lock, a new call to
BaseSession.lock() does not block the process/thread. This way, we
could simply enable session locking by default and re-use the same
locks for file-level locking. This would enable to have file-level
locking even if session-level locking is disabled, while keeping the
number of locks (and lock collisions) low.
Or, we could try to re-evaluate the need for this finite global lock
pool. I guess Grisha implemented this for the sake of performance
(apparently global mutexes can be expensive in some implementations of
the APR). Why not create a distinct lock for each lock name (and thus
exceed the 32 limit) ?
Regards,
Nicolas
On Apr 11, 2005 6:41 AM, Jim Gallacher <ji...@jgassociates.ca> wrote:
> Hello All,
>
> I've attached an alternative version of FileSession.py. A couple of
> changes from the original version:
>
> 1. Uses PythonOption FileSessionDir to determine directory where session
> files will be save. If this option does not exists, it defaults to
> tempfile.tempdir().
>
> 2. Does file locking independent of the session locking. File locking
> always occurs, while session locking is still optional. I'm finding that
> session locking still causes some problems, so I've changed the default
> for session locking to 0.
>
> 3. Fixed a bug in do_cleanup which was calling time(), not time.time()
> and added import time statement.
>
> 4. Added do_cleanup() optimizations. Two parameters now control the
> behaviour of do_cleanup(). The session file mtime can be used to
> determine if the file is a deletion candidate. If it is it can either be
> deleted immediately or unpickled and the true session expiration
> verified. The speedup can be significant for more than 10,000 sessions.
>
> Regards,
> Jim
>
>
>