Keep the current session per request, not per store

docs/api/store.md

@@ -9,8 +9,9 @@ Subclasses must implement:
 | Method | Signature | Description |
 |--------|-----------|-------------|
 | `storage` | `: String` | Backend name (e.g., `"memory"`, `"redis"`) |
-| `current_session` | `: T` | Get current session |
-| `current_session=` | `(T)` | Set current session |
+| `current_session` | `: T` | Get the current request's session (kept per fiber) |
+| `current_session=` | `(T)` | Set the current request's session |
+| `release` | `: Nil` | Forget the current fiber's session; `SessionHandler` calls it when a request ends |
 | `[]` | `(key : String) : T` | Fetch session by ID (raises `SessionNotFoundException`) |
 | `[]?` | `(key : String) : T?` | Fetch session by ID (returns `nil`) |
 | `[]=` | `(key : String, session : T) : T` | Store a session |

spec/request_isolation_spec.cr

@@ -0,0 +1,97 @@
+require "./spec_helper"
+
+private class Probe
+  include HTTP::Handler
+
+  getter seen = [] of String
+  property? yield_mid_request = false
+
+  def call(context : HTTP::Server::Context)
+    Fiber.yield if yield_mid_request?
+    seen << Session.config.store.as(Session::MemoryStore(UserSession)).current_session.session_id
+  end
+end
+
+private def request_with(session_id : String?) : HTTP::Server::Context
+  request = HTTP::Request.new("GET", "/")
+  if session_id
+    request.cookies << HTTP::Cookie.new(name: Session.config.session_key, value: session_id)
+  end
+  HTTP::Server::Context.new(request, HTTP::Server::Response.new(IO::Memory.new))
+end
+
+describe "request isolation" do
+  it "gives a visitor without a cookie a session of their own, not the previous visitor's" do
+    reset_config
+    store = Session::MemoryStore(UserSession).new
+    Session.config.store = store
+    owner = UserSession.new
+    owner.username = "alice"
+    store[owner.session_id] = owner
+
+    handler = Session::SessionHandler(UserSession).new(store)
+    handler.next = Probe.new
+
+    handler.call(request_with(owner.session_id))
+    anonymous = request_with(nil)
+    handler.call(anonymous)
+
+    cookie = anonymous.response.cookies[Session.config.session_key]?
+    cookie.should_not be_nil
+    cookie.try(&.value).should_not eq owner.session_id
+  end
+
+  it "keeps concurrent requests on their own sessions" do
+    reset_config
+    store = Session::MemoryStore(UserSession).new
+    Session.config.store = store
+    alice = UserSession.new
+    bob = UserSession.new
+    store[alice.session_id] = alice
+    store[bob.session_id] = bob
+
+    probe = Probe.new
+    probe.yield_mid_request = true
+    handler = Session::SessionHandler(UserSession).new(store)
+    handler.next = probe
+
+    done = Channel(Nil).new
+    [alice, bob].each do |owner|
+      spawn do
+        handler.call(request_with(owner.session_id))
+        done.send(nil)
+      end
+    end
+    2.times { done.receive }
+
+    probe.seen.sort.should eq [alice.session_id, bob.session_id].sort
+  end
+
+  it "releases the request's session when the request ends, even when it raises" do
+    reset_config
+    store = Session::MemoryStore(UserSession).new
+    Session.config.store = store
+
+    handler = Session::SessionHandler(UserSession).new(store)
+    handler.next = ->(_context : HTTP::Server::Context) do
+      store.current_session
+      raise "boom"
+    end
+
+    expect_raises(Exception, "boom") { handler.call(request_with(nil)) }
+    store.@current_sessions.should be_empty
+  end
+
+  it "does not keep sessions of fibers that have finished" do
+    reset_config
+    store = Session::MemoryStore(UserSession).new
+
+    done = Channel(Nil).new
+    50.times { spawn { store.current_session; done.send(nil) } }
+    50.times { done.receive }
+    Fiber.yield
+
+    store.current_session
+    store.@current_sessions.size.should be < 5
+  end
+end

src/handlers/session_handler.cr

@@ -9,6 +9,8 @@ module Session
       load_session(context)
       call_next(context)
       save_session(context)
+    ensure
+      @session.release
     end
 
     private def load_session(context : HTTP::Server::Context)

src/store.cr

@@ -22,10 +22,29 @@ module Session
     # Storage abstraction - must be implemented by subclasses
     abstract def storage : String
 
-    # Current session must be provided by subclasses
-    # Typically implemented as: property current_session : T = T.new
-    abstract def current_session : T
-    abstract def current_session=(current_session : T)
+    # The session of the request being served. A store is shared by every request, so it is
+    # kept per fiber: concurrent requests, and the next request on a keep-alive connection,
+    # never see each other's session. `SessionHandler` calls `release` when a request ends.
+    #
+    # A fiber spawned during a request starts with a blank session; capture the session
+    # before `spawn` to use it there.
+    def current_session : T
+      @mutex.synchronize { @current_sessions[Fiber.current]? || remember(T.new) }
+    end
+
+    def current_session=(current_session : T)
+      @mutex.synchronize { remember(current_session) }
+    end
+
+    private def remember(session : T) : T
+      @current_sessions.reject! { |fiber, _| fiber.dead? }
+      @current_sessions[Fiber.current] = session
+    end
+
+    # Forget the current fiber's session once its request is done.
+    def release : Nil
+      @mutex.synchronize { @current_sessions.delete(Fiber.current) }
+    end
 
     # Core store operations
     abstract def [](key : String) : T
@@ -37,6 +56,7 @@ module Session
 
     # Instance variables for session lifecycle
     @mutex : Mutex = Mutex.new
+    @current_sessions = {} of Fiber => T
     @flash : Flash = Flash.new
 
     # Access flash messages

src/stores/clustered_redis_store.cr

@@ -26,7 +26,6 @@ module Session
     getter redis_store : RedisStore(T)
     getter coordinator : ClusterCoordinator(T)
     getter circuit_breaker : CircuitBreaker?
-    property current_session : T = T.new
 
     def initialize(@client : Redis = Redis.new, config : ClusterConfig = ClusterConfig.new)
       @redis_store = RedisStore(T).new(@client)

src/stores/cookie_store.cr

@@ -3,7 +3,6 @@ require "../store"
 module Session
   class CookieStore(T) < Store(T)
     include Enumerable(HTTP::Cookie)
-    property current_session : T = T.new
     property cookies : HTTP::Cookies = HTTP::Cookies.new
 
     def initialize(@cookies : HTTP::Cookies = HTTP::Cookies.new)

src/stores/memory_store.cr

@@ -3,7 +3,6 @@ module Session
     include Enumerable(T)
     include QueryableStore(T)
     getter sessions : Hash(String, T)
-    property current_session : T = T.new
 
     def initialize(@sessions : Hash(String, T) = Hash(String, T).new)
     end

src/stores/redis_store.cr

@@ -5,7 +5,6 @@ module Session
   class RedisStore(T) < Store(T)
     include QueryableStore(T)
     getter circuit_breaker : CircuitBreaker?
-    property current_session : T = T.new
 
     @client : Redis?
     @pool : ConnectionPool?