dagolden's collected fixes for sessions #150

Closed
wants to merge 23 commits into
from

Projects

None yet

5 participants

@dagolden
dagolden commented Jan 5, 2013

This pull request fixes almost all of the outstanding issues with sessions that I found.

I may have a few more tests drifting in later, but I think this should be merged so others can use it for further refinements and so additional D2-ready session factories can be written against it.

xdg added some commits Dec 13, 2012
@xdg xdg Delete session data key when value set to undef
Without a way to delete session data, sessions can only grow until
expired.

This patch deletes the key from the session data hash if the value is
set to undef.  Querying a non-existent key returns undef, so deleting a
key has the same effective result as setting it to undef with the added
benefit of shrinking the session.

Documentation of these behaviors and a test are included.
37d62ef
@xdg xdg rename Dancer::Session::* to Dancer::SessionFactory::* 279d876
@xdg xdg Move session_dir default to YAML session class
Session directories are a concept specific to file-based sessions.
It doesn't belong in the Core::Role::Config class.
b56f9ba
@xdg xdg Revise SessionFactory implentation API
Instead of handing off entire session objects to _flush, only the
id and data hashrefs are handed off.  This ensures that session object
attributes are not serialized.

Likewise, _retrieve now needs only to return a data hashref, which
SessionFactory will instantiate into a new object with *current*
session_config settings.  This ensures things like expiration time
are reset correctly.
9ed793e
@xdg xdg Simple SessionFactory should delete during destory af69b9a
@xdg xdg allow session objects to destroy themselves 1b3a2ff
@xdg xdg Move cookie_name into SessionFactory
This eliminates the hard-coded 'dancer.session' cookie name
in Dancer::Core::Context.
2fe527d
@xdg xdg Set session cookie after request, not before
This ensures that cookie based sessions have a chance to flush
data into the session cookie value before the cookie is set.

This also attempts to only set a session cookie if one existed
or if a session has been referenced.
2eb5163
@bigpresh
Member
bigpresh commented Jan 6, 2013

All looks good & sane to me - would like another core dev to approve too though.

Note: Travis CI thinks the build failed, but it only failed on 5.12, the other versions passed all tests; the failure appears to be spurious, the error was:

Executing your  (perlbrew use 5.12) took longer than 3 minutes and was terminated.

... so yeah, ignore the fact Travis reports test failures.

@bigpresh
Member
bigpresh commented Jan 6, 2013

I poked Travis to re-run the tests - all pass now.

@sukria sukria commented on the diff Jan 6, 2013
lib/Dancer/Core/App.pm
@@ -149,8 +149,13 @@ sub session {
# read if a key is provided
return $session->read($key) if @_ == 2;
- # write to the session
- $session->write($key => $value);
+ # write to the session or delete if value is undef
@sukria
sukria Jan 6, 2013 PerlDancer member

Not sure about that, in some cases, maybe the user wants to set undef to a value in the session, no? Not sure...

@dagolden
dagolden Jan 6, 2013

Because reading from a non-existent value still returns undef, functionally it won't matter as long as they are using the Session object API. It only matters if people are checking the underlying data hash ( exists(session->data->{$key} ) and that's sort of an encapsulation violation anyway.

I'm don't feel terribly strongly, about it. session->delete($key) may be sufficient.

@sukria
Member
sukria commented Jan 6, 2013

Thanks for all the work, it needs to be fully tested though, I have a real-life app based on Dancer 2, which uses sessions, at work, I'll test it against this PR before the merge.

Also, we should document that in D2, sessiion engines are named under the SessionFactory namespace now.

xdg added some commits Jan 6, 2013
@xdg xdg improve has_session predicate
Replaces the standard 'has_session' predicate with one that is only true
if a session engine has been defined and either a session has been
retrieved or created or a session cookie was retrieved in the request.

If this is true, calling $context->session returns a session that
previously existed.  If false, calling $context->session creates a new
session (and subsequent has_session() calls are true) or dies if a
session engine is not defined.
745fb39
@xdg xdg Remove creation_time attribute from core Session class
The 'creation_time' attribute is unused and the semantics are not
defined.

Expiration time in the Session class is cookie expiration time,
which may or may not be the same as session expiration, since cookie
expiration is under the control of the user.

If creation_time were a required field that session factories were
mandated to create and preserve on session retrieval, then it could be
used to help establish an independent session duration limit, but this
is not currently the case.

If session duration limits are desired, it is probably better for
applications to develop their own logic using data stored within
the session.

Depending on the nature of the session backend, session factories
could independently track creation time for use in offline
session expiration and disk/memory recovery, but this does not
require an attribute within the core Session class.
1374a35
@xdg xdg Move session cookie management to SessionFactory
This commit moves cookie generation from Dancer::Core::Session
to Dancer::Core::Role::SessionFactory and moves universal
session cookie configuration parameters as well.

The rationale is that SessionFactories should manage implementation
details, such as knowing how cookies are set and retrieved, and should
have configuration that applies to all sessions (such as 'is_secure').

Dancer::Core::Session objects can then purely represent the specific
details of a *specific* session: the ID, the data, and the expiration.
cad9b39
@xdg xdg cleanup Dancer::Core::Session
Groups attributes together, fixes documentation and removes trailing
spaces.
4db982e
@xdg xdg Fix documentation of SessionFactory
Changes POD headers to head2 for consistency.

Clarifies return values required by C<_sessions>.
155c7a5
@dagolden
dagolden commented Jan 7, 2013

I have some better ideas about session destruction and how to avoid proliferating empty sessions. I'll try to get those worked up and committed tomorrow or Tuesday.

@celogeek
celogeek commented Jan 7, 2013

Some tests are missing :

for SessionFactory :

cookie_duration. did you test when a session can expire or not ?

what happen if you can't _flush a session (issue with the disk or anythink), did you test that ? is it good to crash the application in that case ?

did the destroy do properly his job ? did you test it ?

For Dancer::Core::Session :

what happen if you do session->read(undef) ? or session->read ?

same of session->write (without key), is it possible to happen ?

same for delete :)

did you test the destroy ? with the expire ? does it work ?

for Dancer::SessionFactory::Simple :

I see a global vars where you save your data. Did the goal of Dancer2 was to be scoped by module name ? or at least for sharing information, may be the module can call the session of another module ?

did you test the method _retrieve, _sessions, _destroy ?

@sukria
Member
sukria commented Jan 7, 2013

After live-testing that PR:

If the sessions are removed (using the YAML backend) but the client has a cookie:

Exception caught in 'core.app.before_request' filter: Hook error: Can't use an undefined value as a HASH reference at ..../Dancer2/lib/Dancer/Core/Session.pm line 100.

Also, when a session is created, an artifact appears in the session dir:

ls sessions/
a371352d629c232c6938c54ca516a5fdf7742e70.yml  Dancer::Core::Session=HASH(0x56a38d8).yml

These bugs don't exist in master.

@sukria
Member
sukria commented on 1374a35 Jan 7, 2013

I tend to agree with you, but maybe it's better to discuss that before a removal in the code... no? :)

It's easy to put stuff back. I wouldn't have removed it if there was anything in lib/ that used it. :-)

@dagolden
dagolden commented Jan 7, 2013

@sukria, after I revise session destruction, I think those problems may go away. Can you be more specific what you mean about "removed using the YAML backend" and what your before_request hook is doing so I can try to get a test fro it?

@celogeek:

  • most of the lack of test coverage pre-dates my work; I plan to gradually improve tests once my work resolves in my own mind the correct behavior of things. :-)
  • _flush exception crashing the app was preexisting. It seems wise. What else do you suggest? I could imagine a config option that switches the behavior from "die" to "destroy session", but that seems likely to lead to strange errors that aren't noticed for a while. (E.g. user suddenly logged out for no reason.)
  • Dancer::SessionFactory::Simple was merely renamed. It already used a global, which is fine because sessions are global. (E.g. SessionFactory::YAML uses disk as its global.).
@dagolden
dagolden commented Jan 7, 2013

@sukria, I found the .yml artifact, but have a question. Why does SessionFactory::create() call "_flush" and not "flush"? Why do you want to by pass flush hooks if you're actually flushing?

Separately, why not defer flushing new sessions until the end of the request as usual?

xdg added some commits Jan 7, 2013
@xdg xdg flush sessions properly during create() 46b3997
@xdg xdg ensure retrieved sessions have correct data hash a277fd4
@xdg xdg Moved session destruction to context
Sessions are now destroyed via the context object.  This is necessary
because sessions are managed in multiple places and need synchronized
behavior.

In addition to destroying the session via the factory, when a session is
destroyed we need to expire the existing session cookie and send it
back in the response, but only if another session does not replace it.

Likewise, once a session is destroyed, we must ignore the existing
request session cookie and not attempt to retrieve it again.

As a side benefit of this change, Dancer::Core::Session objects no
longer need a link back to their factory, which makes for cleaner
design.

This commit includes more extensive session lifecycle tests.
c2393b4
@dagolden
dagolden commented Jan 8, 2013

@sukria, please review the new destruction commit. I think it's much saner.

@sukria
Member
sukria commented Jan 8, 2013

I'll do today! Thanks for the work! :)

2013/1/8 David Golden notifications@github.com

@sukria https://github.com/sukria, please review the new destruction
commit. I think it's much saner.


Reply to this email directly or view it on GitHubhttps://github.com/PerlDancer/Dancer2/pull/150#issuecomment-11984716.

@sukria
Member
sukria commented Jan 8, 2013

Hey @dagolden, I tested your last changes and it works pretty fine now. I have still a couple of remarks regarding the code, but mostly cosmectic things, I'm going to comment on the revelant parts.

After that, I think we'll be able to merge :)

Thanks a lot for the impressive work on this subject, sessions handling is going to kick ass.

@sukria sukria and 1 other commented on an outdated diff Jan 8, 2013
lib/Dancer/Core/App.pm
return if ! defined $self->context;
- $engine->flush(session => $self->context->session);
+
+ # if a session has been instantiated or we already had a
+ # session, first flush the session so cookie-based sessions can
+ # update the session ID if needed, then set the session cookie
+ # in the response
+
+ my $session;
+ if ( $self->context->has_session ) {
+ $session = $self->context->session;
+ $engine->flush(session => $session);
+ }
+ elsif ( $self->context->_has_destroyed_session ) {
+ $session = $self->context->_destroyed_session;
@sukria
sukria Jan 8, 2013 PerlDancer member

I'm a bit puzzled by the fact that we call a private method from the outside: if we know about _destroyed_session then it should be public, no? This appears elsewhere, I won't comment everywhere but the same remark applies.

@dagolden
dagolden Jan 8, 2013

Oops.

Originally, I didn't have this here because I set the cookie during destroy_session. But tests caught that that sets two session cookies, because they are pushed onto headers instead of replacing. So this code migrated out here and I never made the method public.

I don't really like having to cache a destroyed session and having it public, but it's not the worst thing in the world.

An alternative would be to add a "session_cookie" attribute to Dancer::Core::Response and only push that to the headers in Dancer::Core::Reponse::to_psgi. Then destroy_session in Context could set that on destroy, and the hook in App would overwrite it if a new session exists and the extra _has_destroyed_session clause could go away.

What do you think?

@sukria
sukria Jan 8, 2013 PerlDancer member

I'm not sure we want to alter Dancer::Core::Response with session-specific data (here the "session_cookie" attribute). I think that the Response class should not know about a "session".

It's fine for me if in your current branch, the private methods are renamed with public names.

@sukria sukria and 1 other commented on an outdated diff Jan 8, 2013
lib/Dancer/Core/Context.pm
@@ -114,22 +115,80 @@ sub _build_session {
if ! defined $engine;
# find the session cookie if any
- my $session_id;
- my $session_cookie = $self->cookie('dancer.session');
- if (defined $session_cookie) {
- $session_id = $session_cookie->value;
- }
-
- # if we have a session cookie, try to retrieve the session
- if (defined $session_id) {
- eval { $session = $engine->retrieve(id => $session_id) };
- croak "Fail to retreive session: $@"
- if $@ && $@ !~ /Unable to retrieve session/;
+ unless ( $self->_destroyed_session ) {
@sukria
sukria Jan 8, 2013 PerlDancer member

I really don't like unless blocks, I use unless exclusively for postfixed conditions, because with a block, the temptation of introducing an "else" later on is dangerous... What about unless (condition) { } else { } ?

I would really prefer to see if (! $self->destroyed_session) here (same remark for the private method).

I know these can be subjective remarks, but if we can share common coding guidelines in the core, it's better ;)

BTW @celogeek : do we have a PerlCritic policy implemented with dzil? This kind of stuff should be triggered by our policy.

@dagolden
dagolden Jan 8, 2013

Will fix. And I'll see if I can get dzil tests running to check against policy stuff.

@dagolden
dagolden Jan 8, 2013

FWIW, it's not the only place in the code that qr/unless (/ appears.

@sukria
sukria Jan 8, 2013 PerlDancer member

Yes, but I think it's the only place where unless is used with a block (not
in a postfix condition).

Thanks for the updates!

2013/1/8 David Golden notifications@github.com

In lib/Dancer/Core/Context.pm:

@@ -114,22 +115,80 @@ sub _build_session {
if ! defined $engine;

 # find the session cookie if any
  • my $session_id;
  • my $session_cookie = $self->cookie('dancer.session');
  • if (defined $session_cookie) {
  •    $session_id = $session_cookie->value;
    

- }

  • if we have a session cookie, try to retrieve the session

  • if (defined $session_id) {
  •    eval { $session = $engine->retrieve(id => $session_id) };
    
  •    croak "Fail to retreive session: $@"
    
  •      if $@ && $@ !~ /Unable to retrieve session/;
    
  • unless ( $self->_destroyed_session ) {

FWIW, it's not the only place in the code that qr/unless (/ appears.


Reply to this email directly or view it on GitHubhttps://github.com/PerlDancer/Dancer2/pull/150/files#r2576035.

xdg added some commits Jan 8, 2013
@xdg xdg replace unless block with if not block 5c6c4c6
@xdg xdg add tests for SessionFactory config
Test that session cookies can be modified by engine config settings.
818d8c7
@xdg xdg make private _destroyed_session into public destroyed_session fbe77ac
@xdg xdg improve destroyed_session isa with InstanceOf 25fb2b0
@xdg xdg Tidy session-related changes
While a .perltidyrc exists, most source files were not tidy.  I have
attempted to tidy session-related code I was working on.
b31fb41
@xdg xdg keep test sessions in t/sessions 69ec5f8
@sukria
Member
sukria commented Jan 9, 2013

Merged! Thanks a lot for the awesome work David!

@sukria sukria closed this Jan 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment