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

[Sergio Salvi]

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