Re: [Catalyst] Unnecessary session writes
On 17 Dec 2008, at 13:11, Bill Moseley wrote: On Wed, Dec 17, 2008 at 08:34:36AM +, Tomas Doran wrote: Apologies if I wasn't being clear perviously - could you convert your suggested changes and test into a diff against the distribution which someone could just apply with patch, rather than having to fiddle around copying chunks of code out of email? Sure, but not until we can figure out what changes really need to be made. I think that not creating a session until something has been written into it is a totally sane default (and providing an option to always create one as-per the current behavior if people actually want that). But, the plugin also always writes an expires: key into the store (hence the /^session:/ match above), so if the goal is to not write to the store unless there's something to store then that needs looking at, too. Agreed. But, if you have an application that is mostly read-only on the session then there's the potential of timing out an active session if it's not refreshed. Depends on the application. If using the application naturally writes to the session often then it's not such a problem. I think that for backwards compatibility, we need to keep the session updated on read. However it would be nice to provide a way to configure the session plugin so that reads didn't refresh the session.. And for stores that manage expiry we don't really want that extra store call in addition to session store. Session handling could do with refactoring as-per the authentication plugins, so that the store and state were not plugins themselves, this would make things a lot 'nicer'. However, in the shorter term, providing people with a way to change the default behaviors would go a long way. If you'd like to include TODO tests for that in the patch (or just fix it, with no TODO in the test), then that would be fantastic. As you've probably seen from the mailing list already, there are a couple of pending patches on Catalyst-Plugin-Session already: http://dev.catalyst.perl.org/repos/Catalyst/branches/Catalyst-Plugin- Session/ I have not paid that much attention. What's the plan for those branches? The plan is to try and get some people to actually test them out (both people who have the issues these patches solve, and people who don't), before merging them, and then getting them released.. That's my understanding anyway, I may be wrong: the Session plugin isn't my baby, I'm just trying to ensure the left foot knows what the right foot is doing here. :) Cheers t0m ___ 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/
Re: [Catalyst] Unnecessary session writes
On Mon, Dec 29, 2008 at 06:10:34PM +, Tomas Doran wrote: Session handling could do with refactoring as-per the authentication plugins, so that the store and state were not plugins themselves, this would make things a lot 'nicer'. However, in the shorter term, providing people with a way to change the default behaviors would go a long way. I'll try and find some time to look at it. There's other issues -- I've had a few problems with the session code over time, and discussed often with nothingmuch and posted a few orphaned patches. Two I just came across in the last week are it throwing an exception on invalid session id (instead of just ignoring like a missing one), and the cookie_secure feature that indeed sets the cookie as secure but doesn't prevent it from being sent in a non-SSL session back to the client, kind of defeating the purpose. -- Bill Moseley mose...@hank.org Sent from my iMutt ___ 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/
Re: [Catalyst] Unnecessary session writes
On 17 Dec 2008, at 05:11, Bill Moseley wrote: On Tue, Dec 16, 2008 at 06:20:43PM +, Tomas Doran wrote: Do you fancy writing a test for the issue so we can actually prove it is gone? Well, it would could be something like this: snip Again looks perfectly reasonable to me :) Apologies if I wasn't being clear perviously - could you convert your suggested changes and test into a diff against the distribution which someone could just apply with patch, rather than having to fiddle around copying chunks of code out of email? But, the plugin also always writes an expires: key into the store (hence the /^session:/ match above), so if the goal is to not write to the store unless there's something to store then that needs looking at, too. Agreed. If you'd like to include TODO tests for that in the patch (or just fix it, with no TODO in the test), then that would be fantastic. As you've probably seen from the mailing list already, there are a couple of pending patches on Catalyst-Plugin-Session already: http://dev.catalyst.perl.org/repos/Catalyst/branches/Catalyst-Plugin- Session/ It's easier to merge patches than chunks of changed code, but I think it's pretty open-season on getting niggles fixed in the Session plugin at the moment. :) Cheers t0m ___ 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/
Re: [Catalyst] Unnecessary session writes
On Wed, Dec 17, 2008 at 08:34:36AM +, Tomas Doran wrote: On 17 Dec 2008, at 05:11, Bill Moseley wrote: On Tue, Dec 16, 2008 at 06:20:43PM +, Tomas Doran wrote: Do you fancy writing a test for the issue so we can actually prove it is gone? Well, it would could be something like this: snip Again looks perfectly reasonable to me :) Apologies if I wasn't being clear perviously - could you convert your suggested changes and test into a diff against the distribution which someone could just apply with patch, rather than having to fiddle around copying chunks of code out of email? Sure, but not until we can figure out what changes really need to be made. But, the plugin also always writes an expires: key into the store (hence the /^session:/ match above), so if the goal is to not write to the store unless there's something to store then that needs looking at, too. Agreed. But, if you have an application that is mostly read-only on the session then there's the potential of timing out an active session if it's not refreshed. Depends on the application. If using the application naturally writes to the session often then it's not such a problem. And for stores that manage expiry we don't really want that extra store call in addition to session store. If you'd like to include TODO tests for that in the patch (or just fix it, with no TODO in the test), then that would be fantastic. As you've probably seen from the mailing list already, there are a couple of pending patches on Catalyst-Plugin-Session already: http://dev.catalyst.perl.org/repos/Catalyst/branches/Catalyst-Plugin- Session/ I have not paid that much attention. What's the plan for those branches? It's easier to merge patches than chunks of changed code, but I think it's pretty open-season on getting niggles fixed in the Session plugin at the moment. :) I understand. -- Bill Moseley mose...@hank.org Sent from my iMutt ___ 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/
Re: [Catalyst] Unnecessary session writes
On 10 Dec 2008, at 22:25, Bill Moseley wrote:
When Catalyst::Session fetches an existing session it records its
signature which it then compare with the session data at the end
of the request to decide if the session should be written.
snip
So, if you look at the session every request, for example:
# See if user has selected a language preference
my $language = $c-session-{language} || 'en';
Then if a session doesn't exist it will generate a new session id and
store the empty session to the database (or whatever store you have).
A bot could have fun inserting rows into your database.
That's not cool :(
I'm using this instead:
That looks sensible.
Do you fancy writing a test for the issue so we can actually prove it
is gone?
Cheers
t0m
___
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/
Re: [Catalyst] Unnecessary session writes
On Tue, Dec 16, 2008 at 06:20:43PM +, Tomas Doran wrote:
Do you fancy writing a test for the issue so we can actually prove it is
gone?
Well, it would could be something like this:
use lib t/lib;
use Test::WWW::Mechanize::Catalyst SessionTestApp;
$SessionTestApp::store_session_data = 0;
my $wrote_session;
{
no warnings 'redefine';
*SessionTestApp::store_session_data = sub {
$wrote_session++ if $_[1] =~ /^session:/;
}
}
my $ua = Test::WWW::Mechanize::Catalyst-new;
$ua-get_ok( 'http://localhost/login', 'call $c-session' );
ok( !$wrote_session, 'Did not write session on read only' );
But, the plugin also always writes an expires: key into the store
(hence the /^session:/ match above), so if the goal is to not write to
the store unless there's something to store then that needs looking
at, too.
--
Bill Moseley
mose...@hank.org
Sent from my iMutt
___
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/
