Merge branch 'memcache-session-bugfix'

* memcache-session-bugfix:
  Test-fix for shallow copy change checks
  Added test for deep hash checks, prevent shallow copy check failure
  Session::Memcache fixes
  Updating Session::Memcache test
  Inlining of #merge_sessions

2 files changed, 89 insertions, 56 deletions

diff --git a/lib/rack/session/memcache.rb b/lib/rack/session/memcache.rb
index 4a65cbf3..44629da3 100644
--- a/lib/rack/session/memcache.rb
+++ b/lib/rack/session/memcache.rb
@@ -29,9 +29,13 @@ module Rack
         super
 
         @mutex = Mutex.new
-        @pool = MemCache.
-          new @default_options[:memcache_server], @default_options
-        raise 'No memcache servers' unless @pool.servers.any?{|s|s.alive?}
+        mserv = @default_options[:memcache_server]
+        mopts = @default_options.
+          reject{|k,v| MemCache::DEFAULT_OPTIONS.include? k }
+        @pool = MemCache.new mserv, mopts
+        unless @pool.active? and @pool.servers.any?{|c| c.alive? }
+          raise 'No memcache servers'
+        end
       end
 
       def generate_sid
@@ -41,24 +45,23 @@ module Rack
         end
       end
 
-      def get_session(env, sid)
-        session = @pool.get(sid) if sid
+      def get_session(env, session_id)
         @mutex.lock if env['rack.multithread']
-        unless sid and session
-          env['rack.errors'].puts("Session '#{sid.inspect}' not found, initializing...") if $VERBOSE and not sid.nil?
-          session = {}
-          sid = generate_sid
-          ret = @pool.add sid, session
-          raise "Session collision on '#{sid.inspect}'" unless /^STORED/ =~ ret
+        unless session_id and session = @pool.get(session_id)
+          session_id, session = generate_sid, {}
+          unless /^STORED/ =~ @pool.add(session_id, session)
+            raise "Session collision on '#{session_id.inspect}'"
+          end
         end
-        session.instance_variable_set('@old', {}.merge(session))
-        return [sid, session]
-      rescue MemCache::MemCacheError, Errno::ECONNREFUSED # MemCache server cannot be contacted
-        warn "#{self} is unable to find server."
+        session.instance_variable_set '@old', @pool.get(session_id, true)
+        return [session_id, session]
+      rescue MemCache::MemCacheError, Errno::ECONNREFUSED
+        # MemCache server cannot be contacted
+        warn "#{self} is unable to find memcached server."
         warn $!.inspect
         return [ nil, {} ]
       ensure
-        @mutex.unlock if env['rack.multithread']
+        @mutex.unlock if @mutex.locked?
       end
 
       def set_session(env, session_id, new_session, options)
@@ -66,43 +69,50 @@ module Rack
         expiry = expiry.nil? ? 0 : expiry + 1
 
         @mutex.lock if env['rack.multithread']
-        session = @pool.get(session_id) || {}
         if options[:renew] or options[:drop]
           @pool.delete session_id
           return false if options[:drop]
           session_id = generate_sid
-          @pool.add session_id, 0 # so we don't worry about cache miss on #set
+          @pool.add session_id, {} # so we don't worry about cache miss on #set
         end
-        old_session = new_session.instance_variable_get('@old') || {}
-        session = merge_sessions session_id, old_session, new_session, session
-        @pool.set session_id, session, expiry
-        return session_id
-      rescue MemCache::MemCacheError, Errno::ECONNREFUSED # MemCache server cannot be contacted
-        warn "#{self} is unable to find server."
-        warn $!.inspect
-        return false
-      ensure
-        @mutex.unlock if env['rack.multithread']
-      end
 
-      private
+        session = @pool.get(session_id) || {}
+        old_session = new_session.instance_variable_get '@old'
+        old_session = old_session ? Marshal.load(old_session) : {}
 
-      def merge_sessions sid, old, new, cur=nil
-        cur ||= {}
-        unless Hash === old and Hash === new
-          warn 'Bad old or new sessions provided.'
-          return cur
-        end
+        unless Hash === old_session and Hash === new_session
+          env['rack.errors'].
+            puts 'Bad old_session or new_session sessions provided.'
+        else # merge sessions
+          # alterations are either update or delete, making as few changes as
+          # possible to prevent possible issues.
 
-        delete = old.keys - new.keys
-        warn "//@#{sid}: delete #{delete*','}" if $VERBOSE and not delete.empty?
-        delete.each{|k| cur.delete k }
+          # removed keys
+          delete = old_session.keys - new_session.keys
+          if $VERBOSE and not delete.empty?
+            env['rack.errors'].
+              puts "//@#{session_id}: delete #{delete*','}"
+          end
+          delete.each{|k| session.delete k }
 
-        update = new.keys.select{|k| new[k] != old[k] }
-        warn "//@#{sid}: update #{update*','}" if $VERBOSE and not update.empty?
-        update.each{|k| cur[k] = new[k] }
+          # added or altered keys
+          update = new_session.keys.
+            select{|k| new_session[k] != old_session[k] }
+          if $VERBOSE and not update.empty?
+            env['rack.errors'].puts "//@#{session_id}: update #{update*','}"
+          end
+          update.each{|k| session[k] = new_session[k] }
+        end
 
-        cur
+        @pool.set session_id, session, expiry
+        return session_id
+      rescue MemCache::MemCacheError, Errno::ECONNREFUSED
+        # MemCache server cannot be contacted
+        warn "#{self} is unable to find memcached server."
+        warn $!.inspect
+        return false
+      ensure
+        @mutex.unlock if @mutex.locked?
       end
     end
   end
diff --git a/test/spec_rack_session_memcache.rb b/test/spec_rack_session_memcache.rb
index f21ac3de..faac796e 100644
--- a/test/spec_rack_session_memcache.rb
+++ b/test/spec_rack_session_memcache.rb
@@ -6,11 +6,9 @@ begin
   require 'rack/response'
   require 'thread'
 
-  pool = Rack::Session::Memcache.new(lambda {})
-
   context "Rack::Session::Memcache" do
     session_key = Rack::Session::Memcache::DEFAULT_OPTIONS[:key]
-    session_match = /#{session_key}=[0-9a-fA-F]+;/
+    session_match = /#{session_key}=([0-9a-fA-F]+);/
     incrementor = lambda do |env|
       env["rack.session"]["counter"] ||= 0
       env["rack.session"]["counter"] += 1
@@ -29,22 +27,22 @@ begin
       incrementor.call(env)
     end
 
-    specify "MemCache can connect to existing server" do
-      test_pool = MemCache.new :namespace => 'test:rack:session'
-    end
-
     specify "faults on no connection" do
       if RUBY_VERSION < "1.9"
         lambda do
-          Rack::Session::Memcache.new(incrementor, :memcache_server => '')
+          Rack::Session::Memcache.new incrementor, :memcache_server => 'nosuchserver'
         end.should.raise
       else
         lambda do
-          Rack::Session::Memcache.new(incrementor, :memcache_server => '')
+          Rack::Session::Memcache.new incrementor, :memcache_server => 'nosuchserver'
         end.should.raise ArgumentError
       end
     end
 
+    specify "connect to existing server" do
+      test_pool = MemCache.new incrementor, :namespace => 'test:rack:session'
+    end
+
     specify "creates a new cookie" do
       pool = Rack::Session::Memcache.new(incrementor)
       res = Rack::MockRequest.new(pool).get("/")
@@ -159,6 +157,31 @@ begin
       res3.body.should.equal '{"counter"=>4}'
     end
 
+    specify "deep hashes are correctly updated" do
+      store = nil
+      hash_check = proc do |env|
+        session = env['rack.session']
+        unless session.include? 'test'
+          session.update :a => :b, :c => { :d => :e },
+            :f => { :g => { :h => :i} }, 'test' => true
+        else
+          session[:f][:g][:h] = :j
+        end
+        [200, {}, session.inspect]
+      end
+      pool = Rack::Session::Memcache.new(hash_check)
+      req = Rack::MockRequest.new(pool)
+
+      res0 = req.get("/")
+      session_id = (cookie = res0["Set-Cookie"])[session_match, 1]
+      ses0 = pool.pool.get(session_id, true)
+
+      res1 = req.get("/", "HTTP_COOKIE" => cookie)
+      ses1 = pool.pool.get(session_id, true)
+
+      ses1.should.not.equal ses0
+    end
+
     # anyone know how to do this better?
     specify "multithread: should cleanly merge sessions" do
       next unless $DEBUG
@@ -169,7 +192,7 @@ begin
       res = req.get('/')
       res.body.should.equal '{"counter"=>1}'
       cookie = res["Set-Cookie"]
-      sess_id = cookie[/#{pool.key}=([^,;]+)/,1]
+      session_id = cookie[session_match, 1]
 
       delta_incrementor = lambda do |env|
         # emulate disconjoinment of threading
@@ -191,7 +214,7 @@ begin
         request.body.should.include '"counter"=>2'
       end
 
-      session = pool.pool.get(sess_id)
+      session = pool.pool.get(session_id)
       session.size.should.be tnum+1 # counter
       session['counter'].should.be 2 # meeeh
 
@@ -215,7 +238,7 @@ begin
         request.body.should.include '"counter"=>3'
       end
 
-      session = pool.pool.get(sess_id)
+      session = pool.pool.get(session_id)
       session.size.should.be tnum+1
       session['counter'].should.be 3
 
@@ -237,7 +260,7 @@ begin
         request.body.should.include '"foo"=>"bar"'
       end
 
-      session = pool.pool.get(sess_id)
+      session = pool.pool.get(session_id)
       session.size.should.be r.size+1
       session['counter'].should.be.nil?
       session['foo'].should.equal 'bar'