Ricardo Peres created an issue — 17th February 2013, 11:02:55:
Currently ThreadLocalSessionContext does not inherit from CurrentSessionContext, and as such, it is not possible to use it with the static methods CurrentSessionContext.Bind() and Unbind(). However, unexperienced developers may be fooled into that, and may find themselves in trouble.
If it is considered that this implementation offers advantages over ThreadStaticSessionContext, it would be useful to properly implement it so that it can be used; otherwise, just remove it or mark it as obsolete.
Ricardo Peres added a comment — 20th February 2014, 9:46:28:
This is a quick fix also, just inherit from CurrentSessionContext.
Daniel A. Schilling added a comment — 20th February 2014, 15:00:01:
That wouldn't actually work - the behavior of static methods doesn't change based on what inherits it. ThreadLocalSessionContext.Bind(session)
works one way, while CurrentSessionContext.Bind(session)
works another way - and those differences in behavior are important to those classes. Simply changing ThreadLocalSessionContext
to inherit from CurrentSessionContext
would actually make it more confusing for developers. Since ThreadLocalSessionContext
still needs the behavior contained in its own Bind
method, now it would have two static Bind
methods (one from CurrentSessionContext
), and developers would still have to know call the correct one - ThreadLocalSessionContext.Bind(session)
.
I think it would be better it there weren't these static methods at all. Instead of doing this:
ThreadLocalSessionContext.Bind(session);
session = ThreadLocalSessionContext.Unbind(factory);
... I think this would be better:
session.BindAsCurrentSession();
session = factory.UnbindCurrentSession();
This would require adding Bind
and Unbind
methods to the ICurrentSessionContext
interface (and a HasBind
method would be nice, too), and implementing some new methods on ISession
and ISessionFactory
. But you would no longer have to change which static methods your code called depending on how you configured Configuration.CurrentSessionContext<T>()
- just setting that one configuration value would take care of all the rest.
Daniel A. Schilling added a comment — 20th February 2014, 15:23:11:
The new methods for ISession
and ISessionFactory
could be implemented as extension methods, minimizing the impact of this change. Something like...
public static void BindAsCurrentSession(this ISession session)
{
var factoryImplementor = (ISessionFactoryImplementor)session.SessionFactory;
var context = factoryImplementor.CurrentSessionContext;
context.Bind(session); // TODO: add this new `Bind` method to ICurrentSessionContext
}
public static ISession UnbindCurrentSession(this ISessionFactory factory)
{
var factoryImplementor = (ISessionFactoryImplementor)factory;
var context = factoryImplementor.CurrentSessionContext;
return context.Unbind(factory); // TODO: add this new `Unbind` method to ICurrentSessionContext
}
Ricardo Peres added a comment — 20th February 2014, 17:12:03:
Daniel:
I guess you have never used ThreadLocalSessionContext. I know very well about the Bind method, the problem is when you declare ThreadLocalSessionContext by fluent configuration or .config file: NHibernate throws an exception because the base class is not what it expects.
The static methods are standard on all SessionContext classes, I know they are not inherited, but that's the way they were designed.
Daniel A. Schilling added a comment — 20th February 2014, 17:23:05:
I've been using ThreadLocalSessionContext
for my unit tests and WebSessionContext
for my MVC web application against NHibernate 3.3.3.4001 with no issues. What exception do you receive? Code that expects a specific base class rather than simply expecting something that implements ICurrentSessionContext
is not written correctly.
Ricardo Peres added a comment — 20th February 2014, 17:33:55:
You are binding directly on ThreadLocalSessionContext instead of CurrentSessionContext.
The code that throws the exception is CurrentSessionContext.Bind(session). This allows better decoupling, you do not have to know the exact class you are using.
Daniel A. Schilling added a comment — 20th February 2014, 19:07:57:
Correct, using CurrentSessionContext.Bind(session)
when the context has been configured to be ThreadLocalSessionContext
does not work. But merely changing the inheritance tree will not fix that. Changing ThreadLocalSessionContext
's base class would end up amounting to a complete re-write of ThreadLocalSessionContext
in order to make CurrentSessionContext.Bind(session)
work for it, if it ends up being possible at all.
I think the changes I've suggested (#comment-31861) would be the ideal long-term fix - though they would be breaking changes for anybody that has implemented their own ICurrentSessionContext
. But anybody (myself, for one) attempting to leverage different kinds of ICurrentSessionContexts within the same solution will have to work around this problem, so it would be nice if the fix was baked into NHibernate.
To develop a workaround for this issue, you can think of ICurrentSessionContext
and related classes as an incomplete example of the (http://en.wikipedia.org/wiki/Strategy_pattern) design pattern. You can fill in the missing pieces (Bind and Unbind) to build a completed strategy interface yourself:
public interface ICurrentSessionContextStrategy
{
void Configure(Configuration config);
void Bind(ISession session);
ISession Unbind(ISessionFactory factory);
}
public abstract class BaseCurrentSessionContextStrategy<T> : ICurrentSessionContextStrategy
where T : ICurrentSessionContext, new()
{
public void Configure(Configuration config)
{
config.CurrentSessionContext<T>();
}
public abstract void Bind(ISession session);
public abstract ISession Unbind(ISessionFactory factory);
}
public class CurrentSessionContextStrategy<T> : BaseCurrentSessionContextStrategy<T>
where T : CurrentSessionContext, new()
{
public override void Bind(ISession session)
{
CurrentSessionContext.Bind(session);
}
public override ISession Unbind(ISessionFactory factory)
{
return CurrentSessionContext.Unbind(factory);
}
}
public class ThreadLocalSessionContextStrategy : BaseCurrentSessionContextStrategy<ThreadLocalSessionContext>
{
public override void Bind(ISession session)
{
ThreadLocalSessionContext.Bind(session);
}
public override ISession Unbind(ISessionFactory factory)
{
return ThreadLocalSessionContext.Unbind(factory);
}
}
Then you create a single instance of whichever strategy you want and use it to handle both the configuration and the binding/unbinding.
public static class CurrentSessionContextStrategy
{
public static ICurrentSessionContextStrategy Strategy { get; set; }
}
When configuring NHibernate:
CurrentSessionContextStrategy.Strategy = new ThreadLocalSessionContextStrategy();
var config = new Configuration();
// ... other configuration code...
CurrentSessionContextStrategy.Strategy.Configure(config);
// ... now build your session factory ...
When binding:
var session = factory.OpenSession();
CurrentSessionContextStrategy.Strategy.Bind(session);
When unbinding:
var session = CurrentSessionContextStrategy.Strategy.Unbind(factory);
if (session != null)
session.Dispose();
This isn't exactly the way I'm handling it in my code, but as far as dealing with this particular issue, it's the same general idea.
That's a fair chunk of code for a workaround, and I wish it wasn't necessary. I vote for adding Bind
, Unbind
, and maybe HasBind
directly to the ICurrentSessionContext
interface. Static methods are frequently a source of coupling problems. The best way to fix this high-coupling issue is to make these static methods be instance methods instead. Even my workaround has a static Strategy
property which assumes that every session factory in the currently running application uses the same session context strategy. That's probably a safe assumption, until you run into a scenario where it's not. Static methods are unyielding. Their behavior cannot be changed, so they must be used carefully and judiciously - only in places where you're sure no one needs to swap in different behavior. For CurrentSessionContext.Bind(session)
, that is an incorrect assumption.
Ricardo Peres added a comment — 20th February 2014, 23:28:54:
Daniel:
This thread has grown much more than it's worth! :-)
To cut the story short, all that it takes is:
public class ThreadLocalSessionContext : CurrentSessionContext
{
private ThreadLocal<ISession> _session = new ThreadLocal<ISession>();
public ThreadLocalSessionContext(Engine.ISessionFactoryImplementor factory)
{
}
protected override ISession Session
{
get { return (this._session.Value); }
set { this._session.Value = value; }
}
}
And that's it. No major refactoring, no nothing. We keep things as they are and simply allow ThreadLocalSessionContext to be used like all_the_other context classes.
Ricardo Peres added a comment — 21st February 2014, 11:30:40:
Pull request: #254
Ricardo Peres added a comment — 8th May 2014, 11:35:45:
I think this can be included.
Ricardo Peres added a comment — 13th August 2014, 12:09:09:
New pull request created, as requested: #292
Alexander Zaytsev added a comment — 12th August 2016, 4:40:09:
Move unresolved improvements and new features with minor and trivial priorities to 4.2.0
Frédéric Delaporte added a comment — 13th June 2017, 22:13:48:
ThreadLocalSessionContext
main advantage over ThreadStaticSessionContext
is the support of multiple session factories used within the same context (thread). With NH-4032, it will lose that advantage.
Other points are its ability to open a session "on the fly" instead of yielding null
as all other contextes, and a sort of "auto-cleanup" when user bind a new session without having unbound the previous one. Those abilities do not make much sens without some documented behaviors it does not actually implements: at least auto-closing session on transaction end, and the forbidden usage of session without transactions.
Frédéric Delaporte added a comment — 20th June 2017, 20:24:14:
Now that NH-4032 is merged, do we obsolete that ThreadLocalSessionContext
, or do we finish its implementation?
Alexander Zaytsev added a comment — 14th September 2017, 1:47:17:
Move issues from 4.2 to 5.0
Alexander Zaytsev added a comment — 14th September 2017, 1:57:24:
Move minor and trivial improvements to 5.1