Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

[PATCH] Session fixes: sessions should not be created until written to; and session data should be destroyed on session reset

#4938

This patch resolves two issues with Rails session behavior:

1) Sessions should not be created until written to

The implementation of Rails lazy-loading sessions (starting in Rails 2.3) is suboptimal -- specifically, Rails creates a new session, if one doesn't already exists, as soon as the session hash is accessed. So in a simple case where a session-less user requests an action like this:

def test
  render :text => session[:foo]
end

A new session will be created.

This create-on-read behavior is not ideal. Many Rails applications spread session reads all over the place (for example, by checking flash[:error] in layouts); in these cases, sessions are often created when they're not actually used, which diminishes the benefits of lazy-loading. To make things worse, the facility for disabling session creation on a controller-by-controller basis has been removed (starting in 2.3). So it's very difficult in Rails right now to ensure that sessions are only created when they're actually needed.

This issue is probably not a big deal for applications that use the cookie store -- though even in that case, eager session creation can interfere with page or reverse proxy caching schemes that check HTTP headers -- but for applications that use the server-side stores (ActiveRecord, Memcache), it's inefficient to execute writes and consume space on the server for sessions that are never used.

There are several open issues in Lighthouse related to this eager session-creating behavior:

It seems like a better approach is to create a session only when data is put into the session hash. A read against a non-existent session (i.e., a session associated with a request that contains no session id cookie) should not cause a session to be created. So, for a session-less user:

def test
  session[:foo]         # no session created
  session[:foo] = "bar" # session created!
end

The patch makes this change and includes tests for the new behavior.

2) Session data should be destroyed on session reset

One side-effect of the 2.3 session changes is that, for server-based session stores, the reset_session method does not delete session data from the server. The session cookie is cleared, and a new session hash is created, but the data in the underlying session store is left alone.

Of course, server-based session stores must clean up expired sessions anyway, so these garbage sessions are ultimately purged, but it's surprising and problematic that invalid sessions are not simply deleted. A user who fiddles with his cookies, for example, could start re-using a session that the application thinks has been destroyed.

This patch changes the behavior to the following: when a session is destroyed, the associated session data is removed from the store.

Note that this change requires that session stores implement a #destroy method. I've done so for all the built-in stores -- CookieStore (where destroy is noop), MemCacheStore, and ActiveRecord::SessionStore. (ActiveRecord::SessionStore.destroy simply calls destroy on the ActiveRecord.session_class; implementers such as ActiveRecord::SessionStore::Session and ActiveRecord::SessionStore::SqlBypass are already required, per the docs, to provide a destroy method.)

Reported by Michael Lovitt · June 22nd, 2010 @ 11:33 PM

State: resolved
Milestone: 2.3.9
Assigned to: José Valim José Valim
Importance: Low

Activity

  1. Michael Lovitt
  2. Michael Lovitt
    Michael Lovitt
    • Tag changed from patch to 3.x, patch

    June 22nd, 2010 @ 11:53 PM

  3. Michael Lovitt
  4. Jeremy Kemper
    Jeremy Kemper
    • State changed from new to open

    Great patch, Michael. Could you backport to 2-3-stable as well?

    June 23rd, 2010 @ 08:01 PM

  5. Repository
    Repository

    (from [49f52c3d910c8f183afc3a54ea2ae9667f23085e]) Sessions should not be created until written to and session data should be destroyed on reset.

    [#4938]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
    http://github.com/rails/rails/commit/49f52c3d910c8f183afc3a54ea2ae9...

    June 23rd, 2010 @ 08:02 PM

  6. Michael Lovitt
    Michael Lovitt

    Jeremy, a backported patch for 2-3-stable is attached. Thanks!

    June 24th, 2010 @ 05:02 PM

  7. Jeremy Kemper
  8. Michael Lovitt
    Michael Lovitt

    José Valim tells me that the repro steps for his issue are to store a class in the session when using the cookie store; problems were occurring during deserialization. I'll attempt to repro myself and add some test coverage, and will submit an updated patch (incorporating José's changes) for 2-3-stable.

    June 24th, 2010 @ 08:29 PM

  9. Michael Lovitt
    Michael Lovitt
    • Importance changed from to Low

    José committed a revised fix for the deserialization issue:

    http://github.com/rails/rails/commit/21c99e93883c1cf32474ad65a507e6...

    The specific error condition: the cookie store contains a serialized object of a class that has not yet been loaded into the environment. Auto-loading missing classes is generally handled for all stores within the abstract store, but was failing to happen under certain conditions in the case of the cookie store.

    While adding test coverage for José's fix, I discovered one more broken case: when the cookie store contains a serialized object of an unloaded class, an error occurs if the session id is read before the session data is read. I've attached a patch for this, which also tidies up some of the related code, and includes test coverage for all of the above, so hopefully this behavior won't get broken again.

    To come: a revised 2-3-master patch that includes my original improvements plus these latest fixes and tests.

    June 27th, 2010 @ 08:29 PM

  10. Michael Lovitt
  11. Repository
    Repository

    (from [ebee77a28a7267d5f23a28ba23c1eb88a2d7d527]) Fixed that an ArgumentError is thrown when request.session_options[:id] is read in the following scenario: when the cookie store is used, and the session contains a serialized object of an unloaded class, and no session data accesses have occurred yet. Pushed the stale_session_check responsibility out of the SessionHash and down into the session store, closer to where the deserialization actually occurs. Added some test coverage for this case and others related to deserialization of unloaded types.

    [#4938]

    Signed-off-by: José Valim jose.valim@gmail.com
    http://github.com/rails/rails/commit/ebee77a28a7267d5f23a28ba23c1eb...

    June 27th, 2010 @ 09:39 PM

  12. José Valim
  13. Michael Lovitt
    Michael Lovitt
    • Tag changed from 3.x, patch to 2.3.x, 3.x, patch

    Ok, an updated 2-3-stable patch is attached. It includes all the original session improvements and associated tests, plus the work José and I both did on the cookie store deserialization issues.

    June 28th, 2010 @ 04:52 AM

  14. Santiago Pastorino
    Santiago Pastorino
    • Assigned user set to José Valim

    June 30th, 2010 @ 01:54 AM

  15. José Valim
    José Valim

    Michael, after applying your patch I get three failures on Ruby 1.9.2. Could you please investigate? Thanks a lot!

    June 30th, 2010 @ 12:26 PM

  16. Fotos Georgiadis
    Fotos Georgiadis

    Michael, I saw your comment on #4450 that it's fixed by this patch.

    By reading (and applying) the patch I can't see how.
    Are you planning on fixing that bug as well or did you just misunderstood the case?

    Please also see my comment on #4450. Thanks!

    July 12th, 2010 @ 01:00 AM

  17. Michael Lovitt
    Michael Lovitt

    José: I was unable to reproduce the failures you mentioned. After applying the patch, all actionpack tests pass for me on both Ruby 1.9.2 and 1.8.7.

    But the patch no longer applies cleanly to 2-3-stable, due to some recent unrelated session commits, so I've attached a new one. José, if you still get failures with this patch, could you tell me the specific failures you're seeing as well as the specific 1.9.2 release you're using?

    Fotos: I will take a closer look at #4450 and will respond in the case.

    July 13th, 2010 @ 06:57 PM

  18. Repository
    Repository
    • State changed from open to resolved

    (from [257a29d3cca91913325a8bfef0f06b7ec6c4654b]) Sessions should not be created until written to and session data should be destroyed on reset. [#4938 state:resolved]

    Signed-off-by: José Valim jose.valim@gmail.com
    http://github.com/rails/rails/commit/257a29d3cca91913325a8bfef0f06b...

    July 14th, 2010 @ 07:13 AM

  19. PacoGuzman
    PacoGuzman

    Hi!

    I'm calling 2 times reset_session in the same controller action (in a specific condition), but after update my up from 2.3.8 to 2.3.9, I'm getting the following error:

    undefined method destroy' for {}:Hash

    Inside ActionController::Request#reset_session

    Any advice?

    September 11th, 2010 @ 11:49 AM

  20. Ryan Bigg
    Ryan Bigg
    • Tag cleared.

    Automatic cleanup of spam.

    October 9th, 2010 @ 09:47 PM