Skip to content

Commit

Permalink
Properly reset the session on reset_session
Browse files Browse the repository at this point in the history
  • Loading branch information
steveklabnik committed Aug 31, 2012
1 parent c0b6963 commit 000edbb
Showing 1 changed file with 5 additions and 2 deletions.
7 changes: 5 additions & 2 deletions actionpack/lib/action_dispatch/http/request.rb
Expand Up @@ -227,8 +227,11 @@ def body_stream #:nodoc:
# TODO This should be broken apart into AD::Request::Session and probably
# be included by the session middleware.
def reset_session
session.destroy if session && session.respond_to?(:destroy)
self.session = {}
if session && session.respond_to?(:destroy)
session.destroy
else
self.session = {}

This comment has been minimized.

Copy link
@reinh

reinh Aug 31, 2012

Doesn't this need indifferent access? Wasn't that the reason for rails#7478?

self.session = HashWithIndifferentAccess.new

This comment has been minimized.

Copy link
@steveklabnik

steveklabnik Aug 31, 2012

Author Owner

It is not a HWIA, it's a Rack::Session::Abstract::SessionHash.

Also, in All Real Rails Code, the destroy path should be hit. The {} is because of tests.

This comment has been minimized.

Copy link
@reinh

reinh Aug 31, 2012

Ok, it's a magical HWIA. I still think we should maintain the class invariance.

This comment has been minimized.

Copy link
@steveklabnik

steveklabnik Aug 31, 2012

Author Owner

Sure. I'm trying to make Minimal Changes right now to just fix the bug. RSASH needs some stuff to instantiate it, and I'm not sure that I have access to it here?

This comment has been minimized.

Copy link
@reinh

reinh Aug 31, 2012

You're right: if we're breaking the class invariance then it's happening elsewhere and probably shouldn't be fixed here: the fix ought to collapse this if branch since we will no longer be getting here without a RSASH. Maybe our tests depend on the duck-typing of RSASH to Hash which isn't valid because of #destroy?

This comment has been minimized.

Copy link
@steveklabnik

steveklabnik Aug 31, 2012

Author Owner

Quite possible. I haven't gotten the chance to investigate the tests yet.

This comment has been minimized.

Copy link
@reinh

reinh Aug 31, 2012

Seems fair. I think the type leak is worth finding and fixing. It's causing bugs and unnecessary complexity to deal with the unexpected polymorphism. Wonder where that bugger lives...

end
@env['action_dispatch.request.flash_hash'] = nil
end

Expand Down

0 comments on commit 000edbb

Please sign in to comment.