Skip Menu |

This queue is for tickets about the CGI-Session CPAN distribution.

Report information
The Basics
Id: 21952
Status: resolved
Priority: 0/
Queue: CGI-Session

People
Owner: Nobody in particular
Requestors: fporcher [...] smartech.pf
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



CC: Franck PORCHER <fporcher [...] smartech.pf>
Subject: CGI::Session alters custom data and yield to a wrong behavior(CGI::Session bug report)
Date: Sun, 08 Oct 2006 00:10:36 -1000
To: markstos [...] cpan.org, sherzodr [...] cpan.org, bug-Cgi-Session [...] rt.cpan.org
From: Franck Porcher <fporcher [...] smartech.pf>
Hello Mark & Sherzod I just started yesterday to use CGI::Session (as an embedded component of CGI::Forge ), and would like to report a wrong behavior. ----------------------------------------------------- My environment : CGI::Session : Version 4.14 uname -a : Linux perl.smartech.pf 2.6.16.4 #1 PREEMPT Mon Apr 17 15:12:40 TAHT 2006 i686 AMD Athlon(TM) XP 2800+ unknown GNU/Linux perl -v : This is perl, v5.8.7 built for i386-linux ----------------------------------------------------- My code (summarized) : 1 my $opt_dsn = ... 2 my $cgiquery = ... 3 my $s = CGI::Session->load('driver:file;serializer:default', $cgiquery, $opt_dsn) or die; 4 $s = CGI::Session->new('driver:file;serializer:default', $cgiquery, $opt_dsn) if $s->is_empty(); 5 <END OF PROG> ----------------------------------------------------- The suspicious behavior... - The statement line 3 leads to the creation of a new CGI::Session object, say A, and to the creation of a CGI::Session::Sriver::file driver object, say B. - Statement 4 resets $s, so A & B, no longer reachable are DESTROYed and garbage collected.The destruction of A is caught in CGI/Session.pm to automagically call CGI::Session::flush() which does nothing in this case. A new CGI::Session object is created and assigned to $s, say C, which shares the driver B (see log below). - When the end of program is reached (line 5 above), C is caught to be flushed by DESTROY. B having already disappeared out the scene, a new driver is specially created at this time of death only to allow the flushing (DESTROY >> flush() >> CGI::Session::_driver). This new driver ignores $opt_dsn (for instance Directory => /my/temp), so the flushing creates or updates session files at the wrong place... And it appears that C->{_DRIVER_ARGS} is also gone, ----------------------------------------------------- My analysis of the problem Statement line 3 leads CGI::Session::Driver::new() to physically alter its argument (here $opt_dsn) by turning it into a driver object (bless $opt_dsn, <driver>). So $opt_dsn data is no longer a private custom data structure : it has turned into an object (B) elligible for a premature DESTROY when it goes out of reach after statement 4 resets $s. As such, and unfortunate as it is, the garbage collection of B is also that of $opt_dsn, and that of $s->{_DRIVER_ARGS}, which proves to be unavailable (long gone) when used by CGI::Session::_driver() to create a driver for the late flushing (<driverClass>->new( $self->{_DRIVER_ARGS} ). ------------------- My suggestion My idea is that the custom data, here $opt_dsn, *should not* be altered by the underlying CGI::Session logic. My suggestion to restore a good behavior is to prevent CGI::Session::Driver to turn its argument into an object. This is easily done by patching CGI::Forge::Driver::new() as follows ( *bold* shows the suggested patch, /*bold italic*/ shows my other "perturbations" ) : sub new { /*my ($class, $args) = @_; croak "Invalid argument type passed to driver: " . Dumper($args) if $args && ! ref $args; $args ||= {};*/ # my $self = bless ($args, $class) # wrong : $args is a custom data that shouldn't be altered my $self = bless (*{%$args}*, $class); # Instead make it a shallow-clone, and only alter the clone ! return $self if $self->init(); return $self->set_error( "%s->init() returned false", $class); } I've applied it to my CGI::Session version and the suspicious behavior was removed. Cheers, and good luck. I hope that CGI::Session stays around up & running : it's a fine suite of module. Thanks for contributing it. Franck PORCHER ======================= LOGS========================== /*Statement line 3 ... */Oct 7 16:32:21 perl logger: [CGISESSION::LOAD::1] SESSION: CGI::Session=HASH(0x87808ac) Oct 7 16:32:21 perl logger: [CGISESSION::_driver] SESSION: CGI::Session=HASH(0x87808ac) DRIVER: *DRIVERARGS: _HASH(0x87b3a20)_* Oct 7 16:32:21 perl logger: [DRIVER::INIT] DRIVER:* CGI::Session::Driver::file=_HASH(0x87b3a20)_* DIRECTORY: . Oct 7 16:32:21 perl logger: [CGISESSION::LOAD::2] SESSION: CGI::Session=HASH(0x87808ac) DRIVER: CGI::Session::Driver::file=HASH(0x87b3a20) ==> The 3 lines above show how $opt_dsn (*_HASH(0x87b3a20)_*) is turned into an object (_*CGI::Session::Driver::file=HASH(0x87b3a20)*_) /*Statement lien 4 (rest of $s) ...*/ Oct 7 16:32:21 perl logger: [*CGISESSION::DESTROY*] SESSION: CGI::Session=HASH(0x87808ac) DRIVER: Oct 7 16:32:21 perl logger: [*DRIVER::DESTROY*] DRIVER: _*CGI::Session::Driver::file=HASH(0x87b3a20)*_ ==> these 2 lines show that (blessed)$opt_dsn is DESTROYED prematurately.. -- Franck Porcher, Docteur ès Sciences (Paris VI), Informatique théorique ---------------------------------------------------------------------- SMART Technologies Les solutions intelligentes Service et Ingénierie Informatique Solutions Open Source Linux 1995-2006 : Premier fournisseur en Polynésie française Tél: (689) 550 815 Vini: (689) 711 911 Email: fporcher@smartech.pf Web: www.smartech.pf ---------------------------------------------------------------------- "You can analyze the past but you have to design the future."
Subject: Re: [rt.cpan.org #21952] CGI::Session alters custom data and yield to a wrong behavior(CGI::Session bug report)
Date: Tue, 24 Oct 2006 10:08:41 -0400
To: bug-CGI-Session [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Franck Porcher via RT wrote: Show quoted text
> > I just started yesterday to use CGI::Session (as an embedded component > of CGI::Forge ), and would like to report a wrong behavior.
Frank, Thanks for the thorough report. We appreciate the contribution. It seems like you have a rather clear idea of what the issue is, and what the solution is. It would be ideal if you could express the issue in a Test::More style test case we could add to the test suite. From the code you have below it looks like you are almost there. Then, if you fee confident about the fix, go ahead and submit a patch for the code and docs as needed to completely resolve the issue. Otherwise, someone else will get to this eventually. Thanks! Mark
From: fenlisesi [...] gmail.com
How about the attached attempt at a fix for this? --Ali Isik On Tue Oct 24 10:11:32 2006, mark@summersault.com wrote: Show quoted text
> Franck Porcher via RT wrote:
> > > > I just started yesterday to use CGI::Session (as an embedded component > > of CGI::Forge ), and would like to report a wrong behavior.
> > Frank, > > Thanks for the thorough report. We appreciate the contribution. > > It seems like you have a rather clear idea of what the issue is, and > what the solution is. > > It would be ideal if you could express the issue in a Test::More style > test case we could add to the test suite. From the code you have below > it looks like you are almost there. > > Then, if you fee confident about the fix, go ahead and submit a patch > for the code and docs as needed to completely resolve the issue. > > Otherwise, someone else will get to this eventually. > > Thanks! > > Mark
--- Session-original.pm 2006-11-04 20:45:10.000000000 +0200 +++ Session-fixbug21952.pm 2006-11-06 20:41:36.000000000 +0200 @@ -668,7 +668,17 @@ sub load { # load($dsn, $query, \%dsn_args); - $self->{_DRIVER_ARGS} = $dsn_args if defined $dsn_args; + if (defined $dsn_args) { + if (ref( $dsn_args ) eq 'HASH') { + my $data_href = $self->{_DRIVER_ARGS}; + for my $k (keys %$dsn_args) { + $data_href->{ $k } = $dsn_args->{ $k }; + } + } + else { + ## carp about bad $dsn_args here + } + } }
From: fenlisesi [...] gmail.com
No, this seems to have broken some tests. Please ignore it while I check those out. --Ali Isik On Mon Nov 06 13:42:33 2006, fenlisesi@gmail.com wrote: Show quoted text
> How about the attached attempt at a fix for this? > > --Ali Isik > > On Tue Oct 24 10:11:32 2006, mark@summersault.com wrote:
> > Franck Porcher via RT wrote:
> > > > > > I just started yesterday to use CGI::Session (as an embedded
component Show quoted text
> > > of CGI::Forge ), and would like to report a wrong behavior.
> > > > Frank, > > > > Thanks for the thorough report. We appreciate the contribution. > > > > It seems like you have a rather clear idea of what the issue is, and > > what the solution is. > > > > It would be ideal if you could express the issue in a Test::More style > > test case we could add to the test suite. From the code you have below > > it looks like you are almost there. > > > > Then, if you fee confident about the fix, go ahead and submit a patch > > for the code and docs as needed to completely resolve the issue. > > > > Otherwise, someone else will get to this eventually. > > > > Thanks! > > > > Mark
> >
Subject: Re: [rt.cpan.org #21952] CGI::Session alters custom data and yield to a wrong behavior(CGI::Session bug report)
Date: Mon, 06 Nov 2006 13:51:35 -0500
To: bug-CGI-Session [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Ali ISIK via RT wrote: Show quoted text
> Queue: CGI-Session > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21952 > > > No, this seems to have broken some tests. Please > ignore it while I check those out.
Thanks. In any case, here's a simpler way to make a shallow copy: %$dst = %$src; If we need a deep copy, a cloning module should probably be used. Mark
From: fenlisesi [...] gmail.com
Deep copies are likely to be expensive, though. If deeper structure is allowed within \%dsn_args and this is a rare, non-typical scenario, perhaps we should just put a warning in the docs saying something like "If your %dsn_args has deeper structure in it and you want to use it with multiple constructor calls, you must generate a deep copy of it and use that"? --Ali Isik On Mon Nov 06 13:54:29 2006, mark@summersault.com wrote: Show quoted text
> Ali ISIK via RT wrote:
> > Queue: CGI-Session > > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21952 > > > > > No, this seems to have broken some tests. Please > > ignore it while I check those out.
> > Thanks. In any case, here's a simpler way to make a shallow copy: > > %$dst = %$src; > > If we need a deep copy, a cloning module should probably be used. > > Mark >
Subject: Re: [rt.cpan.org #21952] CGI::Session alters custom data and yield to a wrong behavior(CGI::Session bug report)
Date: Mon, 06 Nov 2006 20:22:50 -0500
To: bug-CGI-Session [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Ali ISIK via RT wrote: Show quoted text
> Queue: CGI-Session > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21952 > > > Deep copies are likely to be expensive, though. If deeper > structure is allowed within \%dsn_args and this is a > rare, non-typical scenario, perhaps we should just put a > warning in the docs saying something like "If your %dsn_args > has deeper structure in it and you want to use it with > multiple constructor calls, you must generate a deep copy > of it and use that"?
I think it's reasonable to just stick with a shallow copy for now. Perhaps somewhere in the notes for driver authors we could add a note about this, but it does seem particular unusual to configure a session module which a such a complex data structure. Mark -- http://mark.stosberg.com/
Subject: Re: [rt.cpan.org #21952] CGI::Session alters custom data and yield to a wrong behavior(CGI::Session bug report)
Date: Mon, 06 Nov 2006 22:32:55 -1000
To: bug-CGI-Session [...] rt.cpan.org
From: Franck Porcher <fporcher [...] smartech.pf>
The shallow copy was the idea I first proposed as a patch along with the discovery of the problem: # ORIGINAL my $self = bless ($args, $class) # $args is a custom data and altered as a CGI::Session:xxx object ! # PROPOSED PATCH (that I called a shallow-clone, which is exactly the same thing proposed by Mark : a shallow copy) my $self = bless (*{%$args}*, $class); # Create a shallow copy of $args should be sufficient in most cases. I've applied this patch to my CGI::Session version and the suspicious behavior was removed. I've been quite happy since 8-) As what you suggest Mark, regarding Test::More, I never used it yet, but could easily make it, along with some description, if we agree with the idea of the proposed patch. Let me know. Cheers, Franck Porcher Ali ISIK via RT wrote: Show quoted text
><URL: http://rt.cpan.org/Ticket/Display.html?id=21952 > > >Deep copies are likely to be expensive, though. If deeper >structure is allowed within \%dsn_args and this is a >rare, non-typical scenario, perhaps we should just put a >warning in the docs saying something like "If your %dsn_args >has deeper structure in it and you want to use it with >multiple constructor calls, you must generate a deep copy >of it and use that"? > >--Ali Isik > >On Mon Nov 06 13:54:29 2006, mark@summersault.com wrote: > >
>>Ali ISIK via RT wrote: >> >>
>>> Queue: CGI-Session >>> Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21952 > >>> >>>No, this seems to have broken some tests. Please >>>ignore it while I check those out. >>> >>>
>>Thanks. In any case, here's a simpler way to make a shallow copy: >> >>%$dst = %$src; >> >>If we need a deep copy, a cloning module should probably be used. >> >> Mark >> >> >>
> > > > >
-- Franck Porcher, Docteur ès Sciences (Paris VI), Informatique théorique ---------------------------------------------------------------------- SMART Technologies Les solutions intelligentes Service et Ingénierie Informatique Solutions Open Source Linux 1995-2006 : Premier fournisseur en Polynésie française Tél: (689) 550 815 Vini: (689) 711 911 Email: fporcher@smartech.pf Web: www.smartech.pf ---------------------------------------------------------------------- "You can analyze the past but you have to design the future."
Subject: CGI::Session alters custom data and yield to a wrong behavior (fixed)
RT-Send-CC: sherzodr [...] cpan.org
This is fixed in SVN now, with a regression test for it. I also tracked own and fixed the SQLite driver bug that Sherzod exposed as part of his work on this. This change will appear in the next release, perhaps Soon, with credit to you. Thanks for the report. Mark