From: Ben P. <be...@pa...> - 2006-04-03 04:22:10
Attachments:
SessionStore.py.patch
|
Hi, I have a bug and fix to submit. Attached is a patch file for WebKit/SessionStore.py against the 0.9 release. The SessionStore convenience functions items() and values() include a bug where a session key can be deleted after self.keys() has been called, but before the session has been accessed from the store. This leads to an unhandled KeyError which causes SessionSweeper to fail, allowing sessions to pile up until catastrophic failure due to lack of memory. When using the SessionMemoryStore, this bug is extraordinarily rare. I only encountered it with a high-traffic site where we use SessionDynamicStore, and every Monday (the site's highest traffic day) the appserver would consume all memory ending up with about 75,000 sessions, and I would find traces of this error in the logs (way back in the logs). After applying the fix a few weeks ago, the app has been fine ever since. Please let me know if I should submit this another way. I wasnt sure if the bug tracking at SourceForge is still in use. Thanks! - Ben |
From: Christoph Z. <ci...@on...> - 2006-04-03 07:39:38
|
> Hi, I have a bug and fix to submit. Attached is a patch file for > WebKit/SessionStore.py against the 0.9 release. Thanks. I will have a look at it and check it in if it's ok. Maybe I will add some tests for the session operations as well. > Please let me know if I should submit this another way. I wasnt sure if > the bug tracking at SourceForge is still in use. The list is the right place. But you can still use the bug tracker to attach your patches and leave a permament note when nobody on the list finds the time to care for it right now. -- Christoph |
From: Christoph Z. <ci...@on...> - 2006-04-07 22:57:15
|
Ben Parker wrote: > Hi, I have a bug and fix to submit. Attached is a patch file for > WebKit/SessionStore.py against the 0.9 release. > > The SessionStore convenience functions items() and values() include a > bug where a session key can be deleted after self.keys() has been > called, but before the session has been accessed from the store. This > leads to an unhandled KeyError which causes SessionSweeper to fail, > allowing sessions to pile up until catastrophic failure due to lack of > memory. > > When using the SessionMemoryStore, this bug is extraordinarily rare. I > only encountered it with a high-traffic site where we use > SessionDynamicStore, and every Monday (the site's highest traffic day) > the appserver would consume all memory ending up with about 75,000 > sessions, and I would find traces of this error in the logs (way back in > the logs). After applying the fix a few weeks ago, the app has been fine > ever since. I have checked this in as suggested, adding a similar fix for get(). For a completely foolproof solution, maybe items(), values() and get() should be overridden in SessionDynamicStore, and wrapped in a lock? -- Christoph |
From: Ben P. <be...@pa...> - 2006-04-07 23:29:26
|
Christoph Zwerschke wrote on 04/07/2006 03:57 PM: > Ben Parker wrote: > >> The SessionStore convenience functions items() and values() include a >> bug where a session key can be deleted after self.keys() has been >> called, but before the session has been accessed from the store. This >> leads to an unhandled KeyError which causes SessionSweeper to fail, >> allowing sessions to pile up until catastrophic failure due to lack >> of memory. > > > I have checked this in as suggested, adding a similar fix for get(). > Thanks, Cristoph! > For a completely foolproof solution, maybe items(), values() and get() > should be overridden in SessionDynamicStore, and wrapped in a lock? > I agree in spirit that every SessionStore should be encouraged to implement it's own version of these functions to ensure correctness and efficiency. But at least now the base class is correct. In practice, I'm not sure wrapping a lock around an items() call in SessionDynamicStore or SessionFileStore is the best idea, because it could take a relatively long time to load all the pickled Sessions from the drive. When I first encountered this, I looked at rewriting those functions at the SessionDynamicStore level, and didn't because I thought they would end up basically the same as the corrected base class version. However, something like a database-backed SessionStore would want to override those methods to be more efficient. - Ben |