the great $c->session_expires refactoring

diff --git a/lib/Catalyst/Plugin/Session.pm b/lib/Catalyst/Plugin/Session.pm
index e5d2af3..625a356 100644 (file)
--- a/lib/Catalyst/Plugin/Session.pm
+++ b/lib/Catalyst/Plugin/Session.pm
@@ -94,16 +94,15 @@ sub _save_session {
     my $c = shift;
 
     if ( my $sid = $c->_sessionid ) {
-        if ( my $session_data = $c->_session ) {
 
-            # all sessions are extended at the end of the request
-            my $now = time;
+        # all sessions are extended at the end of the request
+        my $now = time;
+
+        if ( my $expires = $c->session_expires ) {
+            $c->store_session_data( "expires:$sid" => $expires );
+        }
 
-            # the ref is a workaround for FastMmap:
-            # FastMmap can't store values which aren't refs
-            # this yields errors and other great suckage
-            $c->store_session_data( "expires:$sid" =>
-                  ( { expires => $c->config->{session}{expires} + $now } ) );
+        if ( my $session_data = $c->_session ) {
 
             no warnings 'uninitialized';
             if ( Object::Signature::signature($session_data) ne
@@ -137,41 +136,31 @@ sub _load_session {
     my $c = shift;
 
     if ( my $sid = $c->_sessionid ) {
-        no warnings 'uninitialized';    # ne __address
-
-        # see above for explanation  of the workaround for FastMmap problem
-        my $session_expires =
-          ( $c->get_session_data("expires:$sid") || { expires => 0 } )
-          ->{expires};
+        if ( $c->session_expires ) {    # > 0
 
-        if ( $session_expires < time ) {
+            my $session_data = $c->get_session_data("session:$sid");
+            $c->_session($session_data);
 
-            # session expired
-            $c->log->debug("Deleting session $sid (expired)") if $c->debug;
-            $c->delete_session("session expired");
-            return;
-        }
+            no warnings 'uninitialized';    # ne __address
+            if (   $c->config->{session}{verify_address}
+                && $session_data->{__address} ne $c->request->address )
+            {
+                $c->log->warn(
+                        "Deleting session $sid due to address mismatch ("
+                      . $session_data->{__address} . " != "
+                      . $c->request->address . ")",
+                );
+                $c->delete_session("address mismatch");
+                return;
+            }
 
-        my $session_data = $c->get_session_data("session:$sid");
-        $c->_session($session_data);
+            $c->log->debug(qq/Restored session "$sid"/) if $c->debug;
+            $c->_session_data_sig(
+                Object::Signature::signature($session_data) );
+            $c->_expire_session_keys;
 
-        if (   $c->config->{session}{verify_address}
-            && $session_data->{__address} ne $c->request->address )
-        {
-            $c->log->warn(
-                    "Deleting session $sid due to address mismatch ("
-                  . $session_data->{__address} . " != "
-                  . $c->request->address . ")",
-            );
-            $c->delete_session("address mismatch");
-            return;
+            return $session_data;
         }
-
-        $c->log->debug(qq/Restored session "$sid"/) if $c->debug;
-        $c->_session_data_sig( Object::Signature::signature($session_data) );
-        $c->_expire_session_keys;
-
-        return $session_data;
     }
 
     return;
@@ -212,8 +201,13 @@ sub delete_session {
     $c->delete_session_data("${_}:${sid}") for qw/session expires flash/;
 
     # reset the values in the context object
-    $c->_session(undef);
-    $c->_sessionid(undef);
+    $c->$_(undef) for qw/
+      _sessionid
+      _session
+      _session_expires
+      _session_data_sig
+      /;
+
     $c->_session_delete_reason($msg);
 }
 
@@ -226,6 +220,30 @@ sub session_delete_reason {
     $c->_session_delete_reason(@_);
 }
 
+sub session_expires {
+    my ( $c, $should_create ) = @_;
+
+    $c->_session_expires || do {
+        if ( my $sid = $c->_sessionid ) {
+            my $now = time;
+
+            if ( !$should_create ) {
+                if ( ( $c->get_session_data("expires:$sid") || 0 ) < $now ) {
+
+                    # session expired
+                    $c->log->debug("Deleting session $sid (expired)")
+                      if $c->debug;
+                    $c->delete_session("session expired");
+                    return 0;
+                }
+            }
+
+            return $c->_session_expires(
+                $now + $c->config->{session}{expires} );
+        }
+    };
+}
+
 sub sessionid {
     my $c = shift;
 
@@ -315,6 +333,7 @@ sub create_session_id {
         $c->log->debug(qq/Created session "$sid"/) if $c->debug;
 
         $c->sessionid($sid);
+        $c->session_expires(1);
     }
 }
 

diff --git a/t/lib/SessionTestApp.pm b/t/lib/SessionTestApp.pm
index dede3ce..99c8f00 100644 (file)
--- a/t/lib/SessionTestApp.pm
+++ b/t/lib/SessionTestApp.pm
@@ -22,7 +22,7 @@ sub logout : Global {
 sub page : Global {
     my ( $self, $c ) = @_;
     if ( $c->sessionid ) {
-        $c->res->output("you are logged in");
+        $c->res->output("you are logged in, session expires at " . $c->session_expires);
         $c->session->{counter}++;
     }
     else {

diff --git a/t/live_app.t b/t/live_app.t
index 9abd52a..63f69cb 100644 (file)
--- a/t/live_app.t
+++ b/t/live_app.t
@@ -14,7 +14,7 @@ BEGIN {
       or plan skip_all =>
       "Test::WWW::Mechanize::Catalyst is required for this test";
 
-    plan tests => 30;
+    plan tests => 36;
 }
 
 use lib "t/lib";
@@ -44,10 +44,26 @@ $_->get_ok( "http://localhost/page", "get main page" ) for $ua1, $ua2;
 $ua1->content_contains( "you are logged in", "ua1 logged in" );
 $ua2->content_contains( "you are logged in", "ua2 logged in" );
 
+my ( $u1_expires ) = ($ua1->content =~ /(\d+)$/);
+my ( $u2_expires ) = ($ua2->content =~ /(\d+)$/);
+
+sleep 1;
+
+$_->get_ok( "http://localhost/page", "get main page" ) for $ua1, $ua2;
+
+$ua1->content_contains( "you are logged in", "ua1 logged in" );
+$ua2->content_contains( "you are logged in", "ua2 logged in" );
+
+my ( $u1_expires_updated ) = ($ua1->content =~ /(\d+)$/);
+my ( $u2_expires_updated ) = ($ua2->content =~ /(\d+)$/);
+
+cmp_ok( $u1_expires, "<", $u1_expires_updated, "expiry time updated");
+cmp_ok( $u2_expires, "<", $u2_expires_updated, "expiry time updated");
+
 $ua2->get_ok( "http://localhost/logout", "log ua2 out" );
 $ua2->content_like( qr/logged out/, "ua2 logged out" );
-$ua2->content_like( qr/after 1 request/,
-    "ua2 made 1 request for page in the session" );
+$ua2->content_like( qr/after 2 request/,
+    "ua2 made 2 requests for page in the session" );
 
 $_->get_ok( "http://localhost/page", "get main page" ) for $ua1, $ua2;
 
@@ -56,8 +72,8 @@ $ua2->content_contains( "please login",      "ua2 not logged in" );
 
 $ua1->get_ok( "http://localhost/logout", "log ua1 out" );
 $ua1->content_like( qr/logged out/, "ua1 logged out" );
-$ua1->content_like( qr/after 3 requests/,
-    "ua1 made 3 request for page in the session" );
+$ua1->content_like( qr/after 4 requests/,
+    "ua1 made 4 request for page in the session" );
 
 $_->get_ok( "http://localhost/page", "get main page" ) for $ua1, $ua2;