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

[Sergio Salvi]

On Thu, Oct 16, 2008 at 10:04 PM, Andy Grundman <andy at hybridized.org> wrote:
> 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 =3D $c->finalize_engine;
>>
>> instead of
>>
>>        $c->dispatch;
>>        $status =3D $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 befo=
re
> headers and body are sent out.  Maybe it should extend finalize_body inst=
ead
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: session.patch
Type: text/x-patch
Size: 1175 bytes
Desc: not available
Url : http://lists.scsys.co.uk/pipermail/catalyst-dev/attachments/20081017/=
2f6531b8/session.bin