Here's an interesting fact: Whenever I hit MyApp with an invalid
session id (i.e. one, that isn't in the store), the plugin tries to
insert this exact session id. If two concurrent requests with an
invalid session come in, they both try to (re-)create this invalid
session. This is due to the use of find_or_create(). But shouldn't the
correct approach be:
a) Try to find the session, e.g. $self->model->find({ $self->id_field => $key })
b) If found, use this session because it's still valid.
c) If not found, create() an entirely NEW(!) session, with a new
session-ID and insert it.
Why on earth would we want to insert an invalid session into the
database, just to delete it afterwards because it was invalid and then
create a fresh session? Here's an SQL log that shows what's going on:
BEGIN WORK
BEGIN WORK
SELECT me.id, me.a_session, me.expires FROM sessions2 me WHERE ( me.id
= ? ): 'session:invalid'
SELECT me.id, me.a_session, me.expires FROM sessions2 me WHERE ( me.id
= ? ): 'session:invalid'
INSERT INTO sessions2 ( id) VALUES ( ? ): 'session:invalid'
INSERT INTO sessions2 ( id) VALUES ( ? ): 'session:invalid'
COMMIT
DELETE FROM sessions2 WHERE ( id = ? ): 'session:invalid'
ROLLBACK
DELETE FROM sessions2 WHERE ( id = ? ): 'flash:invalid'
BEGIN WORK
SELECT me.id, me.a_session, me.expires FROM sessions2 me WHERE ( me.id
= ? ): 'session:new-session'
INSERT INTO sessions2 ( id) VALUES ( ? ): 'session:new-session'
COMMIT
UPDATE sessions2 SET a_session = ?, expires = ? WHERE ( id = ? ):
'foo', '1318239160', 'session:new-session'
So, isn't the use of find_or_create() just plain wrong or am I seeing
things here? :)
--Toby
On Sat, Oct 8, 2011 at 2:23 PM, Janne Snabb <sn...@epipe.com> wrote:
> On Sat, 8 Oct 2011, Janne Snabb wrote:
>
>> return $self->model->find_or_create({ $self->id_field => $key })
>
> Just an update:
>
> I discussed about this with mst on irc and we concluded that my
> initial suggestion for the fix is also not correct. It is likely
> to eliminate the SQL error but also likely to cause spurious behaviour
> in the upper layers.
>
> The upper layer (which I have not looked at, is there a call/inheritance
> graph of Catalyst available somewhere? :) should get notified that
> the old session is gone, thus we can not just autovivify it in the
> storage with the old id. Rather a new id should be issued so that
> the upper layers will have a chance of noticing what happened. I
> think that would also eliminate the collisions (as the new id is a
> new random string).
>
> *::Session::Storage::Cache::* is presumambly a better choice than
> DBI/DBIC in the usual case.
>
> --
> Janne Snabb / EPIPE Communications
> sn...@epipe.com - http://epipe.com/
>
> _______________________________________________
> List: Catalyst@lists.scsys.co.uk
> Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
> Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
> Dev site: http://dev.catalyst.perl.org/
>
_______________________________________________
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
