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

[Andy Grundman]

Sergio Salvi wrote:
> Continuing this thread from the catalyst list - original message at
> http://www.mail-archive.com/catalyst@lists.scsys.co.uk/msg04185.html
> 
> On Wed, Sep 24, 2008 at 12:05 PM, Matt S Trout <dbix-class at trout.me.uk> 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.