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

[Sergio Salvi]

On Fri, Oct 17, 2008 at 5:04 PM, Sergio Salvi <sergio.lists at salvi.ca> wrote:
> On Thu, Oct 16, 2008 at 10:04 PM, Andy Grundman <andy at hybridized.org> wro=
te:
>> 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::Sessi=
on
>> to find another place to save its data, so it can make sure to do it bef=
ore
>> headers and body are sent out.  Maybe it should extend finalize_body ins=
tead
>> of finalize?  If there is absolutely no way to do that, maybe changing t=
he
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: session.patch
Type: text/x-patch
Size: 2286 bytes
Desc: not available
Url : http://lists.scsys.co.uk/pipermail/catalyst-dev/attachments/20081030/=
41b6338c/session.bin