GoogleCodeExporter
commented
8 years ago Hi Matthew,
Sorry that it took so long for us to take action on this issue, and thanks a
lot for the very detailed explanations.
This is indeed a problem in SSP, though a very tough one to solve (and PHP
itself doesn't help here). The Session class that's being problematic is
already a singleton. The problem here is the script you are using is basically
creating one instance of the class per session stored in the memcache server,
which is intuitively wrong when we are dealing with a singleton. How is that
possible, then? Because the singleton is something we define programatically,
but PHP doesn't know about it. Therefore, each time you unserialize the object
from the server, PHP is calling SimpleSAML_Session::__construct(), even though
that's private.
In such situation, we end up with as many session objects as keys in the
servers. The right approach would be to explicitly clean every object after
each iteration in the "main" loop of the script, something like:
foreach($keys as $key) {
$res = SimpleSAML_Memcache::get($key);
if($res === NULL) {
$skipped += 1;
} else {
$sync += 1;
}
unset($res);
}
What's the problem then? Well, that'll never work because what unset() does is
just to remove the reference ($res) to the object, but since the object is a
singleton, there's more references pointing to it, one inside the object
itself. This means we are creating objects that we cannot explicitly destroy,
because they are singletons. It doesn't make any sense at all to destroy a
singleton, and in fact there's no way to do it in PHP. The GC will get rid of
it only after the script ends, which is too late for this to work.
So in the end: yes, there's a memory leak in here, and one tough to get rid of,
to be honest.
The solution that that the RH engineer is suggesting is, indeed, very dirty,
because it implies modifying the behaviour of one "generic" class
(SimpleSAML_Session) depending on who is using it in a module. Not to mention
the use of a global variable, which is never a good idea.
The right way to solve this is to, first, stop registering a shutdown function
each time we unserialize a session (it's a singleton, so that should only
happen in the private constructor); and second, stop creating new objects each
time we unserialize a session. In such cases, when we are unserializing we
should fetch the instance, and overwrite its values with those we are restoring.
I've implemented this by making the SimpleSAML_Session class to implement the
Serializable interface, then adding both serialize() and unserialize()
functions, and getting rid of the __wakeup() function that was creating
trouble. This is quite a big change, and could create trouble in some ways, but
I've tried it and both the SP and the IdP keep working, and your bug disappears.
Without the patch and a limit of 10M memory, this is what I get if I run the
script after automatically creating some thousands of sessions:
$ time ./memcacheSync.php
Server localhost:11211 has 3276 items.
Connecting to: localhost:11211
Connected. Finding keys.
Found 3276 key(s).
Total number of keys: 3276
Total number of unique keys: 3276
Starting synchronization.
PHP Fatal error: Allowed memory size of 10485760 bytes exhausted (tried to
allocate 5215 bytes) in
/media/sf_Devel/ssp-repos/idp/lib/SimpleSAML/Memcache.php on line 50
PHP Stack trace:
PHP 1. {main}() /media/sf_Devel/ssp-repos/idp/bin/memcacheSync.php:0
PHP 2. SimpleSAML_Memcache::get()
/media/sf_Devel/ssp-repos/idp/bin/memcacheSync.php:63
PHP 3. unserialize()
/media/sf_Devel/ssp-repos/idp/lib/SimpleSAML/Memcache.php:50
real 0m0.589s
user 0m0.012s
sys 0m0.284s
If I apply the patch, it runs smoothly (and quite fast, I would say):
$ time ./memcacheSync.php
Server localhost:11211 has 4460 items.
Connecting to: localhost:11211
Connected. Finding keys.
Found 4460 key(s).
Total number of keys: 4460
Total number of unique keys: 4460
Starting synchronization.
Synchronization done.
4460 keys in sync.
real 0m1.064s
user 0m0.020s
sys 0m0.808s
I'm attaching the patch here for your convenience, though I've applied this in
trunk too (r3356). Please test it and tell us if you have any problems with it.Original comment by jaim...@gmail.com on 4 Feb 2014 at 3:08
- Changed state: Fixed
- Added labels: Type-Defect
Original issue reported on code.google.com by
matthew.sloweon 15 Nov 2013 at 11:20