From 50fc8eb0f1c35b9d0b8b4b83b603ae15c2fe0c18 Mon Sep 17 00:00:00 2001 From: Scytrin dai Kinthra Date: Sun, 22 Nov 2009 18:08:53 -0800 Subject: Inlining of #merge_sessions --- lib/rack/session/memcache.rb | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/lib/rack/session/memcache.rb b/lib/rack/session/memcache.rb index 4a65cbf3..7ae8707a 100644 --- a/lib/rack/session/memcache.rb +++ b/lib/rack/session/memcache.rb @@ -74,7 +74,22 @@ module Rack @pool.add session_id, 0 # 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 + + begin # merge sessions + unless Hash === old_session and Hash === new_session + warn 'Bad old_session or new_session sessions provided.' + return session + end + + delete = old_session.keys - new_session.keys + env['rack.errors'].puts("//@#{session_id}: delete #{delete*','}") if $VERBOSE and not delete.empty? + delete.each{|k| session.delete k } + + update = new_session.keys.select{|k| new_session[k] != old_session[k] } + env['rack.errors'].puts("//@#{session_id}: update #{update*','}") if $VERBOSE and not update.empty? + update.each{|k| session[k] = new_session[k] } + end + @pool.set session_id, session, expiry return session_id rescue MemCache::MemCacheError, Errno::ECONNREFUSED # MemCache server cannot be contacted @@ -84,26 +99,6 @@ module Rack ensure @mutex.unlock if env['rack.multithread'] end - - private - - 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 - - delete = old.keys - new.keys - warn "//@#{sid}: delete #{delete*','}" if $VERBOSE and not delete.empty? - delete.each{|k| cur.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] } - - cur - end end end end -- cgit v1.2.3-24-ge0c7 From 9cd37ca45503c1d1e6432e95d5444328ab4d6015 Mon Sep 17 00:00:00 2001 From: Scytrin dai Kinthra Date: Sun, 22 Nov 2009 20:12:04 -0800 Subject: Updating Session::Memcache test Pointless instantiation removed Moved bad connection check above good connection check A blank string for the server specification uses defaults, fixed --- test/spec_rack_session_memcache.rb | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/test/spec_rack_session_memcache.rb b/test/spec_rack_session_memcache.rb index f21ac3de..4b72a2f9 100644 --- a/test/spec_rack_session_memcache.rb +++ b/test/spec_rack_session_memcache.rb @@ -6,8 +6,6 @@ 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]+;/ @@ -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("/") -- cgit v1.2.3-24-ge0c7 From 2a79d6ff0921ee4193fcd3739df604808c0adce1 Mon Sep 17 00:00:00 2001 From: Scytrin dai Kinthra Date: Sun, 22 Nov 2009 20:15:28 -0800 Subject: Session::Memcache fixes Restructing logical branches to be less inlince Uniform naming of variables Fix of of inline session merging --- lib/rack/session/memcache.rb | 70 ++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/lib/rack/session/memcache.rb b/lib/rack/session/memcache.rb index 7ae8707a..7f48b360 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,30 @@ module Rack end end - def get_session(env, sid) - session = @pool.get(sid) if sid + def get_session(env, session_id) + session = @pool.get(session_id) if 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? + unless session_id and session + if $VERBOSE and not session_id.nil? + env['rack.errors']. + puts "Session '#{session_id.inspect}' not found, initializing..." + end session = {} - sid = generate_sid - ret = @pool.add sid, session - raise "Session collision on '#{sid.inspect}'" unless /^STORED/ =~ ret + session_id = generate_sid + ret = @pool.add session_id, session + unless /^STORED/ =~ ret + 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." + 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) @@ -75,29 +85,39 @@ module Rack end old_session = new_session.instance_variable_get('@old') || {} - begin # merge sessions - unless Hash === old_session and Hash === new_session - warn 'Bad old_session or new_session sessions provided.' - return session - 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. + # removed keys delete = old_session.keys - new_session.keys - env['rack.errors'].puts("//@#{session_id}: delete #{delete*','}") if $VERBOSE and not delete.empty? + if $VERBOSE and not delete.empty? + env['rack.errors']. + puts "//@#{session_id}: delete #{delete*','}" + end delete.each{|k| session.delete k } - update = new_session.keys.select{|k| new_session[k] != old_session[k] } - env['rack.errors'].puts("//@#{session_id}: update #{update*','}") if $VERBOSE and not update.empty? + # 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 @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." + 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 env['rack.multithread'] + @mutex.unlock if @mutex.locked? end end end -- cgit v1.2.3-24-ge0c7 From ed3db52a936624b15c5b8d69aa7f3005dec48240 Mon Sep 17 00:00:00 2001 From: Scytrin dai Kinthra Date: Thu, 3 Dec 2009 13:04:16 -0800 Subject: Added test for deep hash checks, prevent shallow copy check failure Rewording variables for clarity --- test/spec_rack_session_memcache.rb | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/test/spec_rack_session_memcache.rb b/test/spec_rack_session_memcache.rb index 4b72a2f9..faac796e 100644 --- a/test/spec_rack_session_memcache.rb +++ b/test/spec_rack_session_memcache.rb @@ -8,7 +8,7 @@ begin 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 @@ -157,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 @@ -167,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 @@ -189,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 @@ -213,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 @@ -235,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' -- cgit v1.2.3-24-ge0c7 From e89ef8f95389c4d95b1f02a7e64b29ae09b4763d Mon Sep 17 00:00:00 2001 From: Scytrin dai Kinthra Date: Thu, 3 Dec 2009 13:07:46 -0800 Subject: Test-fix for shallow copy change checks Simplification of new/missing session keys --- lib/rack/session/memcache.rb | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/lib/rack/session/memcache.rb b/lib/rack/session/memcache.rb index 7f48b360..44629da3 100644 --- a/lib/rack/session/memcache.rb +++ b/lib/rack/session/memcache.rb @@ -46,21 +46,14 @@ module Rack end def get_session(env, session_id) - session = @pool.get(session_id) if session_id @mutex.lock if env['rack.multithread'] - unless session_id and session - if $VERBOSE and not session_id.nil? - env['rack.errors']. - puts "Session '#{session_id.inspect}' not found, initializing..." - end - session = {} - session_id = generate_sid - ret = @pool.add session_id, session - 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)) + session.instance_variable_set '@old', @pool.get(session_id, true) return [session_id, session] rescue MemCache::MemCacheError, Errno::ECONNREFUSED # MemCache server cannot be contacted @@ -76,14 +69,16 @@ 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 = @pool.get(session_id) || {} + old_session = new_session.instance_variable_get '@old' + old_session = old_session ? Marshal.load(old_session) : {} unless Hash === old_session and Hash === new_session env['rack.errors']. -- cgit v1.2.3-24-ge0c7