Sessions #700

Closed
wants to merge 24 commits into
from

5 participants

@csainty

Time to make a PR out of this stuff and collaborate on cleaning up the last bits of it. I don't believe it is production ready just yet, but it is close and GH seems the easiest place to discuss it.

There is now a single ISessionStore interface that all session mechanisms will be based on.

There is a CookieBasedSessionStore which should be backwards compatible with the existing version, same cookie name and encryption/hash format.

Then there is the AbstractIdBasedSessionStore which handles creating, storing and reading a session id in a cookie and lets the implementer handle persistence using that id.
Can optionally handle the id creation process as well, default it just a Guid.
There is a single concrete implementation of this class based on ConcurrentDictionary called InProcessSessionStore.
I expect Sql,Raven,Redis,Mongo etc could all use this abstract class as their starting point and focus on the persistence rather than the HTTP/Nancy interactions.
Simple enough to just start from ISessionStore if you do need more control though :)

It will default to the CookieBased store at the moment, to change implementations you just hook into the container in your bootstrapper and create a new ISessionStore registration with your desired config / lifetime.
Most implementations are going to need more than just a new() level of config and have varying lifetime requirements, so seemed best to either default to CookieBased or a null based like is currently done, rather than try pick one with the scanner.

Issues to resolve

  1. I don't like the class DynamicSession.cs Its goal is to add basic change tracking to the DynamicDictionary, but the way it is implemented is pretty awful.
    My preference would probably be to expose a delegate or event on the DynamicDictionary to receive update notifications, then the session code can wire it up when it creates the instance it needs. This involves more complex changes to the existing class though, so want an opinion before I go hacking into it too far.

  2. Serialization. I need to take a closer look at the serialization of the DynamicDictionary. I have at least one little work around in place for it, but I need to see if it is a more general problem with a more general fix. The problem is that the serializer does not convert the DynamicDictionaryItem into it's value before serialization.

  3. Docs. Will need to update wiki, and some more xml docs are probably needed as well.

@csainty

Ok I tidied up serialization a little in commit 28d9fb4 by adding a method to DynamicDictionary to get back a plain IDictionary that unwraps the DynamicDictionaryValues

Commit 37247e6 I moved the wiring up of the SessionStore to the NancyContext from the NancyContextFactory to the NancyEngine, feels a little more correct though they both still feel a little wrong to me, just that feeling like I am missing something :)

The basic cache stuff is now in here, I hadn't added it to this branch earlier as I wanted to focus on sessions and how to wire them in properly before adding another dependency to the mix.

A little thought needs giving to the ICacheStore and ISessionStore interfaces, mostly in terms of other functionality we want to make default. At the moment they are plain object stores but there are some things along the lines of timeouts etc that could be added at the base level if we wanted to.

@csainty

Some more code to consider, a quick RavenDB implementation of the session and cache stores to see what one would look like.

https://github.com/csainty/Nancy.Stores.RavenDB

@thecodejunkie
Custodians of the Super-Duper-Happy-Path member

Turning my attention to this now. Will be one of my major focuses for 0.14, apart from async

@csainty

Rebased :)

@phillip-haydon
Custodians of the Super-Duper-Happy-Path member

I think this needs a hook somewhere to help clear data for an expired session.

This may mean running a background task in Nancy which will nuke the data if it's older than a defined time period.

@csainty

Yeah that is a discussion I have been hoping to have, whether we try control that process in Nancy or leave it to the session provider.

Some potential providers, like Mongo, have built in support for expiring documents
http://docs.mongodb.org/manual/tutorial/expire-data/

So if we build in support at the Nancy level for controlling session lifetime, I would want to make sure it can be bypassed by an implementation that can do it "better".
@thecodejunkie and I are trying hard to organise a coding night together to really push on with this stuff, we just keep missing each other.

@grumpydev
Custodians of the Super-Duper-Happy-Path member

Yeah, if we put the expiry stuff into nancy then it needs to be optional, or at least have a "null" expiry provider - although I'm generally more of a fan of the GetOrAdd with a delegate style for caches, which generally negates the need for automatic expiry.

@phillip-haydon
Custodians of the Super-Duper-Happy-Path member

I figured it would be something like:

Sessions.Enable<RedisSessionProvider>();
Sessions.EnableExpiry<RedisSessionExpirerProvider>();

So if MongoDB doesn't need it, no need to register it.

It would only enable the Nancy handler if it was given a provider.

@thecodejunkie
Custodians of the Super-Duper-Happy-Path member

A more appropriate API (and one which we use in several places) would be something like

var provider = 
   new ReditSessionProvider("username", "password", whatever);

Sessions.Enable(provider);

because the likelihood that the different providers are going to need different types of config is very high. Expiry management does sound like it's a provider concern and if the provider supports it, then it should be up to that provider to offer the unnecessary configuration when you create it. No?

@csainty

That was the pattern I was thinking @thecodejunkie

Building expiry into Nancy really only helps the provider developers as far as I see, makes it something they can just implement a couple of interface methods and forget. From the user perspective I don't think it really makes a difference, you configure it in your bootstrapper and it doesn't matter if it is on Nancy or the provider you are creating.

So I think provider developers could but just as easily helps with something like a starter kit, or just a nice clean supported provider like MS SQL that can be used as a template.

@phillip-haydon
Custodians of the Super-Duper-Happy-Path member

If a relational database is being used. None of them have the ability to time a timeout on content. This is a feature only supported by Redis and MongoDB as far as I know. I don't even think Couch supports it.

If someone wants to create their own session persistence I don't think we should rely on them KNOWING they have to handle clearing the cache, I think we should provide them some way to say "implement this interface, and we will invoke it for you to clear the cache"

A template would work too but I think it's nicer for Nancy to handle it, that way with authentication, if we clear the session we have something to invoke to tell it to go clear the session.

If it's not handled by nancy and solely relied on that the persistence handle it, then we can't invoke nancy to clear it without creating a new session id for that user.

csainty and others added some commits Jul 16, 2012
@csainty csainty Added a DynamicSession which extends DynamicDictionary with change tr…
…acking abilities. Very limited to begin with, will be made smarter later.
7ec642b
@csainty csainty Removed old session code that will no longer be supported.
Build is broken at this commit.
de7c582
@csainty csainty Added constructor to DynamicSession to allow it to be init'd with dat…
…a that doesn't register as changes.
954d278
@csainty csainty Expose an ISessionStore via NancyContext, to be created by the NancyC…
…ontextFactory.

NancyModule and Request continue to expose the same session object for easy access from either.
ee5d75a
@csainty csainty Added an ISessionStore interface that can handle saving and loading a…
… session.

It works at a low level with the NancyContext and Request so that it can be used both for client and server side storage.
f0ad176
@csainty csainty Added ApplicationStartup task for Sessions which registers Before and…
… After hooks to handle the loading and saving of sessions.

Sessions will always be enabled in the future defaulting to one of the "works by default" stores.
a6bd7f9
@csainty csainty Added ISessionStore information to NancyInternalConfiguration and Con…
…figurableBootstrapper.

NancyContextFactory is currently handling adding the SessionStore to the NancyContext.
f9711fe
@csainty csainty Diagnostics always using cookie based sessions. 6428909
@csainty csainty Fixing tests that relied on previous version of CookieBasedSessions 5896a20
@csainty csainty Always use CookieBasedSessionStore by default. We are going to have m…
…ultiple built in providers and it is the best bet.
4d378f6
@csainty csainty Added an AbstractIdBasedCookieStore which can be used as a base for y…
…our typical session storage.

Also implemented a very simple InProcessSessionStore based on the AbstractId base and ConcurrentDictionary.
9a58b1e
@csainty csainty Tidy up some merge artefacts 5f6572b
csainty Added docs to the ISessionStore interface 4c34e3b
csainty Added the basic classes for an ICacheStore and InProcessCacheStore. 0af7f17
csainty Removed object serializer parameter from the AbstractIdBasedSessionSt…
…ore. It is not needed at the abstraction level and individual providers can implement it if they need to do serialization of sessions.
a87b0c2
csainty Added ICacheStore to the InternalConfiguration and ConfigurableBootst…
…rapper
9293114
Chris Sainty Moved the code that sets the ISessionStore instance on the NancyConte…
…xt from the NancyContextFactory to the NancyEngine.
222245d
Chris Sainty NancyEngine now set the Cache property on NancyContext. 781ca75
Chris Sainty Tidied up passing the session values into the sessionstore to be save…
…d, now unwrapping the DynamicDictionaryValues so that the serializers in the session store do not need to worry about them.
c78548c
Chris Sainty Added missing documentation and tidied up a few pieces of code. 73b8f1f
@csainty csainty Added a demo session project for quick end to end testing and getting…
… a better feel of how it works from sonsumer side. Is it super duper happy?
df91382
@csainty csainty Fixed build issues after rebase 8706ba1
@csainty csainty Fixed bad merge 4107eaf
@csainty csainty Ignore VS2012 test runner results 79aab19
@phillip-haydon
Custodians of the Super-Duper-Happy-Path member

Ok so I did a bit of research while on holiday, and Ayende just replied on twitter, turns out that RavenDB has a bundle for expiration.

MongoDB, Redis, and RavenDB all support expiration of content. So I take back my suggestion of having a background task to expire content and we deal with NoSQL solutions for handling the session data.

Also if the user wants to use SQL Server. They can run a scheduled task (express) or a sql server task to clean up stale data.

@damianh
Custodians of the Super-Duper-Happy-Path member

I think this is effectively dead. The sentiment in the general community is that we shouldn't be supporting or encouraging sessions; at an aspinsider aspnet vNext presentation the question was put forth and the overall response was a giant 'fuck no!'. Ultimately, somebody somewhere, for better or worse, will do some owin session middleware ( https://www.nuget.org/packages/Owin.RedisSession/ ), and maybe that will suffice for those that want to run with scissors.

Closing now, if you disagree, re-open :)

@damianh damianh closed this Jun 12, 2014
@damianh damianh removed this from the Future milestone Jun 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment