NH-3402 - Refactor ThreadLocalSessionContext #292
Conversation
|
Something weird is happening, because the code compiles perfectly on my machine |
|
try to compile with ShowBuildMenu and choose "TeamCity" there |
|
d:\BuildAgent-03\work\f20cfe335cc08c35\src\NHibernate.Test\ConnectionTest\ThreadLocalCurrentSessionTest.cs(82,11): error CS0103: The name 'context' does not exist in the current context There seems to be a test that inherits from ThreadLocalSessionContext which needs to be rewritten. |
|
Oskar: you are right, but this test is marked to be ignored...? |
|
Don't know why it was marked ignored. Probably the rewritten test will be simpler. The old ThreadLocalSessionContext seems to worry about creating sessions and then being responsible for closing them, which none of the other ICurrentSessionContext implementations cared about. I wonder what the impact of this change will be... Could there be code out there that with this change would require changes in many places (everywhere a new thread or threadpool work item is created) to add session open and close? Since it will not be in 4.0 anymore, a safe and easy way would be to provide the new implementation under a different name and mark the old as deprecated. If we have it in 4.1, this would better follow the semantic versioning principle. I think I prefer this approach - thoughts? |
|
Oskar, |
|
@rjperes tests still fail |
|
I'm looking at it. |
Removed dependency on SharpTestEx.
There was a problem hiding this comment.
The xml doc is obsolete with your change and need an update.
We have to agree on changing the ThreadLocalSessionContext semantic to that point. It was heavily deviating from CurrentSessionContext on one crucial point: trying to get CurrentSession while none are current was causing thread local context to build a session, while the current context throws an exception instead.
Additionally, some "fool" protections were in place for releasing previous session if the user was binding a new one without unbinding the previous one.
Users who were relying on this "on the fly session building" behavior will have their applications fail after upgrade.
Since 5.0 is for doing all breaking changes we deem necessary, why not. It will realign this context with the behavior of all the others, which is a good thing in my opinion.
Otherwise, we would have to leave the previous implementation "as is", preferably marked obsolete, and add a ThreadLocalSessionContext2 (sorry, no idea for a better name).
| @@ -20,164 +19,26 @@ namespace NHibernate.Context | |||
| /// has been called. If <tt>Close()</tt> is called on a session managed by | |||
| /// this class, it will be automatically unbound. | |||
| /// <p/> | |||
| /// Additionally, the static <see cref="Bind"/> and <see cref="Unbind"/> methods are | |||
| /// Additionally, the static <see cref="CurrentSessionContext.Bind"/> and <see cref="CurrentSessionContext.Unbind"/> methods are | |||
| /// provided to allow application code to explicitly control opening and | |||
| /// closing of these sessions. This, with some from of interception, | |||
| /// is the preferred approach. It also allows easy framework integration | |||
| /// and one possible approach for implementing long-sessions. | |||
| /// <p/> | |||
| /// </summary> | |||
There was a problem hiding this comment.
With this change, most of this xml comment is obsolete. The previous implementation was generating a session if there was no current session, which this new implementation does not.
The comment should be updated accordingly.
Worst, this comment looks obsolete from the start. The mechanism for auto-unbinding a closed session seems to have never been implemented though documented in this xml doc. Same for the mechanism which was supposed to render the session unusable till a transaction is started.
The Ignore on his test class confirms it: Ignore("Not yet supported. Need AutoClosed feature.(TransactionContext)"). This ThreadLocalCurrentSessionTest test class looks quite crappy: it is like it assumes OpenSession on factory use the session context, it overrides a method pointlessly (called neither by base implementation nor by the test class), ... It should be deleted I think.
| using NHibernate; | ||
| using NHibernate.Engine; | ||
|
|
||
| namespace NHibernate.Context | ||
| { | ||
| //TODO: refactoring on this class. Maybe using MapBasedSessionContext. |
There was a problem hiding this comment.
Good move to have ignored and removed this comment, since the MapBasedSessionContext implementation is flawed (thread safety issues, going to Jira that), and unneeded with your implementation.
|
Another changes may be missing:
|
|
With #644, this PR in its current state would just change |
NH-3402