diff --git a/docs/api/store.md b/docs/api/store.md index 9877d0e..91d8003 100644 --- a/docs/api/store.md +++ b/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 | diff --git a/spec/request_isolation_spec.cr b/spec/request_isolation_spec.cr new file mode 100644 index 0000000..e0e43e1 --- /dev/null +++ b/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 diff --git a/src/handlers/session_handler.cr b/src/handlers/session_handler.cr index 59675e0..cd00555 100644 --- a/src/handlers/session_handler.cr +++ b/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) diff --git a/src/store.cr b/src/store.cr index 7faea02..617101b 100644 --- a/src/store.cr +++ b/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 diff --git a/src/stores/clustered_redis_store.cr b/src/stores/clustered_redis_store.cr index 70ef5d5..2b0278e 100644 --- a/src/stores/clustered_redis_store.cr +++ b/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) diff --git a/src/stores/cookie_store.cr b/src/stores/cookie_store.cr index ffae345..7ca56f9 100644 --- a/src/stores/cookie_store.cr +++ b/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) diff --git a/src/stores/memory_store.cr b/src/stores/memory_store.cr index c393644..19c9749 100644 --- a/src/stores/memory_store.cr +++ b/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 diff --git a/src/stores/redis_store.cr b/src/stores/redis_store.cr index 0e3d0a4..029d1fc 100644 --- a/src/stores/redis_store.cr +++ b/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?