Mailing List Archive: Catalyst: Dev

Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too)

Index | Next | Previous | View Threaded

sergio.lists at salvi

Oct 16, 2008, 12:28 PM

Post #1 of 7 (2136 views)
Permalink

Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too)

Continuing this thread from the catalyst list - original message at
http://www.mail-archive.com/catalyst [at] lists/msg04185.html

On Wed, Sep 24, 2008 at 12:05 PM, Matt S Trout <dbix-class [at] trout> wrote:
> On Wed, Sep 10, 2008 at 06:59:21PM -0400, Sergio Salvi wrote:
>> There is a race condition in C::P::Session when using
>> C::Engine::Apache (and probably other engines too):
>>
>> I have a simple controller action (let's call it /save) that gets data
>> submitted from an HTML form via POST, process that request, stores
>> some stuff in the session and flash and then redirects with HTTP 303
>> to another action (/display).
>>
>> The /display action then displays the regular "submit successful"
>> message that was set on the previous action by using $c->flash. The
>> problem is that the browser is GETting /display before /save is
>> finished storing the session and flash rows in the database. Then, of
>> course, /display thinks nothing has happened and doesn't display the
>> data from flash.
>>
>> After a bunch of debugging and stack traces :), I figured out the
>> problem is that C::P::Session's finalize() calls $c->NEXT::finalize()
>> before calling $c->finalize_session, so
>> C::Engine::Apache->finalize_body() gets executed *before* the session
>> is flushed in the database, making the browser access /display even
>> though the session may not be stored yet:
>
> This was changed by Bill Moseley in order to fix a bunch of other bugs.
>
> Have a look at the ChangeLog - maybe we could provide an option to reverse
> the order or find another approach?
>

I've checked the ChangeLog and I don't think providing an option to
reverse the order is a safe thing to do. It would break other stuff
and that's why that change was made in the first place.

I couldn't figure out a way to make the Engine finalize() method be
called last, so maybe we should have a specific finalize() method for
Catalyst Engines? Then in Catalyst::handle_request() we do this:

$c->dispatch;
$c->finalize;
$status = $c->finalize_engine;

instead of

$c->dispatch;
$status = $c->finalize;

?

This would also require changing finalize_body, finalize_uploads,
finalize_headers and finalize_cookies so they don't call
$c->engine->finalize_xxx, as the $c->engine->finalize_xxx methods
would be called by $c->finalize_engine.

The finalize_engine method would end up looking almost the same as the
current finalize(), but all method calls would be
$c->engine->finalize_xxx.

How stupid is my idea? Thoughts?

Regards,
Sergio Salvi

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

andy at hybridized

Oct 16, 2008, 7:04 PM

Post #2 of 7 (2011 views)
Permalink

Re: Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too) [In reply to]

Sergio Salvi wrote:
> Continuing this thread from the catalyst list - original message at
> http://www.mail-archive.com/catalyst [at] lists/msg04185.html
>
> On Wed, Sep 24, 2008 at 12:05 PM, Matt S Trout <dbix-class [at] trout> wrote:
>> On Wed, Sep 10, 2008 at 06:59:21PM -0400, Sergio Salvi wrote:
>>> There is a race condition in C::P::Session when using
>>> C::Engine::Apache (and probably other engines too):
>>>
>>> I have a simple controller action (let's call it /save) that gets data
>>> submitted from an HTML form via POST, process that request, stores
>>> some stuff in the session and flash and then redirects with HTTP 303
>>> to another action (/display).
>>>
>>> The /display action then displays the regular "submit successful"
>>> message that was set on the previous action by using $c->flash. The
>>> problem is that the browser is GETting /display before /save is
>>> finished storing the session and flash rows in the database. Then, of
>>> course, /display thinks nothing has happened and doesn't display the
>>> data from flash.
>>>
>>> After a bunch of debugging and stack traces :), I figured out the
>>> problem is that C::P::Session's finalize() calls $c->NEXT::finalize()
>>> before calling $c->finalize_session, so
>>> C::Engine::Apache->finalize_body() gets executed *before* the session
>>> is flushed in the database, making the browser access /display even
>>> though the session may not be stored yet:
>> This was changed by Bill Moseley in order to fix a bunch of other bugs.
>>
>> Have a look at the ChangeLog - maybe we could provide an option to reverse
>> the order or find another approach?
>>
>
> I've checked the ChangeLog and I don't think providing an option to
> reverse the order is a safe thing to do. It would break other stuff
> and that's why that change was made in the first place.
>
> I couldn't figure out a way to make the Engine finalize() method be
> called last, so maybe we should have a specific finalize() method for
> Catalyst Engines? Then in Catalyst::handle_request() we do this:
>
> $c->dispatch;
> $c->finalize;
> $status = $c->finalize_engine;
>
> instead of
>
> $c->dispatch;
> $status = $c->finalize;
>
> ?
>
> This would also require changing finalize_body, finalize_uploads,
> finalize_headers and finalize_cookies so they don't call
> $c->engine->finalize_xxx, as the $c->engine->finalize_xxx methods
> would be called by $c->finalize_engine.
>
> The finalize_engine method would end up looking almost the same as the
> current finalize(), but all method calls would be
> $c->engine->finalize_xxx.
>
> How stupid is my idea? Thoughts?
>
> Regards,
> Sergio Salvi

I see what you mean. I think the best solution would be for
C::P::Session to find another place to save its data, so it can make
sure to do it before headers and body are sent out. Maybe it should
extend finalize_body instead of finalize? If there is absolutely no way
to do that, maybe changing the engine flow like you propose is something
we can think about.

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

sergio.lists at salvi

Oct 17, 2008, 2:04 PM

Post #3 of 7 (2015 views)
Permalink

Re: Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too) [In reply to]

On Thu, Oct 16, 2008 at 10:04 PM, Andy Grundman <andy [at] hybridized> wrote:
> Sergio Salvi wrote:
>>
>> Continuing this thread from the catalyst list - original message at
>> http://www.mail-archive.com/catalyst [at] lists/msg04185.html
>>
>> On Wed, Sep 24, 2008 at 12:05 PM, Matt S Trout <dbix-class [at] trout>
>> wrote:
>>>
>>> On Wed, Sep 10, 2008 at 06:59:21PM -0400, Sergio Salvi wrote:
>>>>
>>>> There is a race condition in C::P::Session when using
>>>> C::Engine::Apache (and probably other engines too):
>>>>
>>>> I have a simple controller action (let's call it /save) that gets data
>>>> submitted from an HTML form via POST, process that request, stores
>>>> some stuff in the session and flash and then redirects with HTTP 303
>>>> to another action (/display).
>>>>
>>>> The /display action then displays the regular "submit successful"
>>>> message that was set on the previous action by using $c->flash. The
>>>> problem is that the browser is GETting /display before /save is
>>>> finished storing the session and flash rows in the database. Then, of
>>>> course, /display thinks nothing has happened and doesn't display the
>>>> data from flash.
>>>>
>>>> After a bunch of debugging and stack traces :), I figured out the
>>>> problem is that C::P::Session's finalize() calls $c->NEXT::finalize()
>>>> before calling $c->finalize_session, so
>>>> C::Engine::Apache->finalize_body() gets executed *before* the session
>>>> is flushed in the database, making the browser access /display even
>>>> though the session may not be stored yet:
>>>
>>> This was changed by Bill Moseley in order to fix a bunch of other bugs.
>>>
>>> Have a look at the ChangeLog - maybe we could provide an option to
>>> reverse
>>> the order or find another approach?
>>>
>>
>> I've checked the ChangeLog and I don't think providing an option to
>> reverse the order is a safe thing to do. It would break other stuff
>> and that's why that change was made in the first place.
>>
>> I couldn't figure out a way to make the Engine finalize() method be
>> called last, so maybe we should have a specific finalize() method for
>> Catalyst Engines? Then in Catalyst::handle_request() we do this:
>>
>> $c->dispatch;
>> $c->finalize;
>> $status = $c->finalize_engine;
>>
>> instead of
>>
>> $c->dispatch;
>> $status = $c->finalize;
>>
>> ?
>>
>> This would also require changing finalize_body, finalize_uploads,
>> finalize_headers and finalize_cookies so they don't call
>> $c->engine->finalize_xxx, as the $c->engine->finalize_xxx methods
>> would be called by $c->finalize_engine.
>>
>> The finalize_engine method would end up looking almost the same as the
>> current finalize(), but all method calls would be
>> $c->engine->finalize_xxx.
>>
>> How stupid is my idea? Thoughts?
>>
>> Regards,
>> Sergio Salvi
>
> I see what you mean. I think the best solution would be for C::P::Session
> to find another place to save its data, so it can make sure to do it before
> headers and body are sent out. Maybe it should extend finalize_body instead
> of finalize? If there is absolutely no way to do that, maybe changing the
> engine flow like you propose is something we can think about.

Yay! Moving finalize_session to finalize_body solved it! Well, I
haven't run the whole test suite yet, but all my (manual) tests
passed. Patch attached.

I have tried moving it to finalize_headers and it worked fine too, but
in order to mimic the original behaviour ("finalize session at the
*very* end"), having it at finalize_body is the closest we can get.

Thanks,
Sergio Salvi

Attachments:

session.patch (1.15 KB)

sergio.lists at salvi

Oct 30, 2008, 1:38 PM

Post #4 of 7 (1983 views)
Permalink

Re: Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too) [In reply to]

On Fri, Oct 17, 2008 at 5:04 PM, Sergio Salvi <sergio.lists [at] salvi> wrote:
> On Thu, Oct 16, 2008 at 10:04 PM, Andy Grundman <andy [at] hybridized> wrote:
>> Sergio Salvi wrote:
>>>
>>> Continuing this thread from the catalyst list - original message at
>>> http://www.mail-archive.com/catalyst [at] lists/msg04185.html
>>>
>>> On Wed, Sep 24, 2008 at 12:05 PM, Matt S Trout <dbix-class [at] trout>
>>> wrote:
>>>>
>>>> On Wed, Sep 10, 2008 at 06:59:21PM -0400, Sergio Salvi wrote:
>>>>>
>>>>> There is a race condition in C::P::Session when using
>>>>> C::Engine::Apache (and probably other engines too):
>>>>>
>>>>> I have a simple controller action (let's call it /save) that gets data
>>>>> submitted from an HTML form via POST, process that request, stores
>>>>> some stuff in the session and flash and then redirects with HTTP 303
>>>>> to another action (/display).
>>>>>
>>>>> The /display action then displays the regular "submit successful"
>>>>> message that was set on the previous action by using $c->flash. The
>>>>> problem is that the browser is GETting /display before /save is
>>>>> finished storing the session and flash rows in the database. Then, of
>>>>> course, /display thinks nothing has happened and doesn't display the
>>>>> data from flash.
>>>>>
>>>>> After a bunch of debugging and stack traces :), I figured out the
>>>>> problem is that C::P::Session's finalize() calls $c->NEXT::finalize()
>>>>> before calling $c->finalize_session, so
>>>>> C::Engine::Apache->finalize_body() gets executed *before* the session
>>>>> is flushed in the database, making the browser access /display even
>>>>> though the session may not be stored yet:
>>>>
>>>> This was changed by Bill Moseley in order to fix a bunch of other bugs.
>>>>
>>>> Have a look at the ChangeLog - maybe we could provide an option to
>>>> reverse
>>>> the order or find another approach?
>>>>
>>>
>>> I've checked the ChangeLog and I don't think providing an option to
>>> reverse the order is a safe thing to do. It would break other stuff
>>> and that's why that change was made in the first place.
>>>
>>> I couldn't figure out a way to make the Engine finalize() method be
>>> called last, so maybe we should have a specific finalize() method for
>>> Catalyst Engines? Then in Catalyst::handle_request() we do this:
>>>
>>> $c->dispatch;
>>> $c->finalize;
>>> $status = $c->finalize_engine;
>>>
>>> instead of
>>>
>>> $c->dispatch;
>>> $status = $c->finalize;
>>>
>>> ?
>>>
>>> This would also require changing finalize_body, finalize_uploads,
>>> finalize_headers and finalize_cookies so they don't call
>>> $c->engine->finalize_xxx, as the $c->engine->finalize_xxx methods
>>> would be called by $c->finalize_engine.
>>>
>>> The finalize_engine method would end up looking almost the same as the
>>> current finalize(), but all method calls would be
>>> $c->engine->finalize_xxx.
>>>
>>> How stupid is my idea? Thoughts?
>>>
>>> Regards,
>>> Sergio Salvi
>>
>> I see what you mean. I think the best solution would be for C::P::Session
>> to find another place to save its data, so it can make sure to do it before
>> headers and body are sent out. Maybe it should extend finalize_body instead
>> of finalize? If there is absolutely no way to do that, maybe changing the
>> engine flow like you propose is something we can think about.
>
> Yay! Moving finalize_session to finalize_body solved it! Well, I
> haven't run the whole test suite yet, but all my (manual) tests
> passed. Patch attached.
>
> I have tried moving it to finalize_headers and it worked fine too, but
> in order to mimic the original behaviour ("finalize session at the
> *very* end"), having it at finalize_body is the closest we can get.
>

Ok, so I've fixed t/03_flash.t make it pass (just renaming c->finalize
to c->finalize_body).

Test 06_finalize.t should to be deleted as it doesn't make sense anymore.

Can you please bless this patch and release a new version to CPAN?

Thanks,
Sergio Salvi

Attachments:

session.patch (2.23 KB)

dbix-class at trout

Nov 12, 2008, 11:19 AM

Post #5 of 7 (1907 views)
Permalink

Re: Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too) [In reply to]

Patch looks sane enough but I'm worried about interaction with other code.

If you can send me an htpasswd line at mst <at> shadowcat.co.uk I can create you
a branch in svn and then you can comit there, and we can let people test.

--
Matt S Trout Need help with your Catalyst or DBIx::Class project?
Technical Director http://www.shadowcat.co.uk/catalyst/
Shadowcat Systems Ltd. Want a managed development or deployment platform?
http://chainsawblues.vox.com/ http://www.shadowcat.co.uk/servers/

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

sergio.lists at salvi

Dec 10, 2008, 6:43 AM

Post #6 of 7 (1767 views)
Permalink

Re: Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too) [In reply to]

On Thu, Dec 4, 2008 at 2:12 PM, Matt Pitts <mpitts [at] a3its> wrote:
> I think this might be affecting my application as well - HTTP::Prefork
> *and* FastCGI instances using memcached for cache/session storage.
>
> I'm getting random "duplicate key" failures when trying to write the
> sessionid to the Cart table and other strange session behavior. It just
> started happening a few weeks ago and I've done a lot of troubleshooting
> on it - thought it might be caused by this session save issue.
>
> Anybody know of the status of this?

My apologies for the delay. I'll get my session patch committed today,
which will *not* fix the duplicate key issue -- that's another patch
I'm working on which will store "flash" inside the session row.

Sergio

>
> _______________________________________________
> Catalyst-dev mailing list
> Catalyst-dev [at] lists
> http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev
>

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

sergio.lists at salvi

Dec 11, 2008, 8:51 AM

Post #7 of 7 (1757 views)
Permalink

Re: Re: [Catalyst] Race condition in Catalyst::Plugin::Session and Catalyst::Engine::Apache (possibly other engines too) [In reply to]

On Wed, Dec 10, 2008 at 9:43 AM, Sergio Salvi <sergio.lists [at] salvi> wrote:
> On Thu, Dec 4, 2008 at 2:12 PM, Matt Pitts <mpitts [at] a3its> wrote:
>> I think this might be affecting my application as well - HTTP::Prefork
>> *and* FastCGI instances using memcached for cache/session storage.
>>
>> I'm getting random "duplicate key" failures when trying to write the
>> sessionid to the Cart table and other strange session behavior. It just
>> started happening a few weeks ago and I've done a lot of troubleshooting
>> on it - thought it might be caused by this session save issue.
>>
>> Anybody know of the status of this?
>
> My apologies for the delay. I'll get my session patch committed today,
> which will *not* fix the duplicate key issue -- that's another patch
> I'm working on which will store "flash" inside the session row.
>
> Sergio
>

Committed: http://dev.catalyst.perl.org/svnweb/Catalyst/browse/branches/Catalyst-Plugin-Session/

I'll soon merge the two patches into a single patch.

I've been successfully using the finalize_race_condition patch in
production for about 2 months. The flash inside session patch is brand
new, but all tests passed :)

Please try it out and let me know how it works for you.

Sergio

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads