NHibernateDataContextAdvanced.StateSession not thread save

GoogleCodeExporter commented 9 years ago

What steps will reproduce the problem?
1. Have multiple threads simultaneously call 
NHibernateDataContext.Advanced.DefaultSession.GetNamedQuery()
2. Ensure that no previous session has been created by a previous call.
Sorry that I post no code/unit test.

What is the expected output? What do you see instead?
Exception:
Object reference not set to an instance of an object.
--Stack trace: (System.NullReferenceException)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at CodeSmith.Data.NHibernate.NHibernateDataContext.NHibernateDataContextAdvanced.get_StateSession() in C:\Work\Libs\PLINQO-NHibernate-1.2\Source\CodeSmith.Data.NHibernate\DataContext\NHibernateDataContext.cs:line 372
   at CodeSmith.Data.NHibernate.NHibernateDataContext.NHibernateDataContextAdvanced.get_DefaultSession() in C:\Work\Libs\PLINQO-NHibernate-1.2\Source\CodeSmith.Data.NHibernate\DataContext\NHibernateDataContext.cs:line 414
   at MyCompany.DataAccess.WorkflowDataContext.GetMyData1(Guid myId) in C:\Work\MyCompany\MyProj1\Services\mpro.Workflow.DataAccess\WorkflowDataContext.generated.cs:line 241

What version of the product are you using?
v1.2 (the concerned code is the same in the HEAD revision of the SVN repository 
(2906).

Please provide any additional information below.
The same problem exists for StatelessStateSession.
I fixed this, see the attached patch.

Original issue reported on code.google.com by christia...@gmail.com on 19 Nov 2012 at 11:05

Attachments:

NHibernateDataContext.cs.patch

GoogleCodeExporter commented 9 years ago

I just realized that the NHibernate session is not thread save either, so 
PLINQO must cache sessions per thread.
Making the private session dictionaries [ThreadStatic] should do the trick.
Here the updated patch.

Original comment by christia...@gmail.com on 19 Nov 2012 at 1:51

Attachments:

NHibernateDataContext.cs.patch

GoogleCodeExporter commented 9 years ago

Thanks for these patches. I'll take a look at them and get them committed.

Original comment by bniemyjski on 19 Nov 2012 at 9:00

Changed state: Started
Added labels: Type-Defect, Framework-NHibernate

GoogleCodeExporter commented 9 years ago

Hello,

I finally had some time to look at your patch and I believe it is the correct 
change to make after reading this: 
http://nhibernate.hibernatingrhinos.com/tags/patterns (search for ThreadStatic)

However, I pulled down the framework samples (in the Framework-Samples branch) 
and added a Tracker unit test to try and break this and I couldn't.

Here is the test that I used:

        [Test]
        public void ThreadSafeGetNamedQueryTest()
        {
            using (var db = new TrackerDataContext()) {
                var t1 = System.Threading.Tasks.Task.Factory.StartNew(() => db.GetUsersWithRoles());
                var t2 = System.Threading.Tasks.Task.Factory.StartNew(() => db.GetUsersWithRoles());
                var t3 = System.Threading.Tasks.Task.Factory.StartNew(() => db.GetUsersWithRoles());
                var users = db.GetUsersWithRoles();

                System.Threading.Tasks.Task.WaitAll(t1, t2, t3);

                Assert.Greater(users.Count, 0);
            }
        }

For reference, the method that it's calling does this:

  public IList<User> GetUsersWithRoles()
        {
            IQuery query = Advanced.DefaultSession.GetNamedQuery("GetUsersWithRoles");

            return query.List<User>();
        }

Is there any chance, you can create a unit test that can break this so I can 
verify your changes?

Original comment by bniemyjski on 27 Nov 2012 at 4:18

Changed state: AwaitingInfo

GoogleCodeExporter commented 9 years ago

I managed to run your test and it *sometimes* failed:

Tracker.Tests.ThreadSafeGetNamedQueryTest:
System.AggregateException : One or more errors occurred.
  ----> NHibernate.Exceptions.GenericADOException : could not execute query
[ exec [dbo].[GetUsersWithRoles] ]
[SQL: exec [dbo].[GetUsersWithRoles]]
  ----> System.InvalidOperationException : There is already an open DataReader associated with this Command which must be closed first.
at System.Threading.Tasks.Task.WaitAll(Task[] tasks, Int32 millisecondsTimeout, 
CancellationToken cancellationToken)
at System.Threading.Tasks.Task.WaitAll(Task[] tasks, Int32 millisecondsTimeout)
at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
at Tracker.Tests.ThreadSafeGetNamedQueryTest() in 
C:\Test\PLINQO-NH-Samples\CSharp\Tracker\Tests.cs:line 243
--GenericADOException
at NHibernate.Loader.Loader.DoList(ISessionImplementor session, QueryParameters 
queryParameters)
at NHibernate.Loader.Loader.ListIgnoreQueryCache(ISessionImplementor session, 
QueryParameters queryParameters)
at NHibernate.Loader.Loader.List(ISessionImplementor session, QueryParameters 
queryParameters, ISet`1 querySpaces, IType[] resultTypes)
at NHibernate.Loader.Custom.CustomLoader.List(ISessionImplementor session, 
QueryParameters queryParameters)
at NHibernate.Impl.SessionImpl.ListCustomQuery(ICustomQuery customQuery, 
QueryParameters queryParameters, IList results)
at NHibernate.Impl.SessionImpl.List(NativeSQLQuerySpecification spec, 
QueryParameters queryParameters, IList results)
at NHibernate.Impl.SessionImpl.List[T](NativeSQLQuerySpecification spec, 
QueryParameters queryParameters)
at NHibernate.Impl.SqlQueryImpl.List[T]()
at Tracker.Data.TrackerDataContext.GetUsersWithRoles() in 
C:\Test\PLINQO-NH-Samples\CSharp\Tracker\Data\TrackerDataContext.generated.cs:li
ne 214
at Tracker.Tests.<>c__DisplayClassb.<ThreadSafeGetNamedQueryTest>b__7() in 
C:\Test\PLINQO-NH-Samples\CSharp\Tracker\Tests.cs:line 240
at System.Threading.Tasks.Task`1.InvokeFuture(Object futureAsObj)
at System.Threading.Tasks.Task.Execute()
--InvalidOperationException
at 
System.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlC
ommand command)
at System.Data.SqlClient.SqlCommand.ValidateCommand(String method, Boolean 
async)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior 
cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method, 
DbAsyncResult result)
at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior 
cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method)
at System.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior, 
String method)
at System.Data.SqlClient.SqlCommand.ExecuteDbDataReader(CommandBehavior 
behavior)
at System.Data.Common.DbCommand.System.Data.IDbCommand.ExecuteReader()
at NHibernate.AdoNet.AbstractBatcher.ExecuteReader(IDbCommand cmd)
at NHibernate.Loader.Loader.GetResultSet(IDbCommand st, Boolean 
autoDiscoverTypes, Boolean callable, RowSelection selection, 
ISessionImplementor session)
at NHibernate.Loader.Loader.DoQuery(ISessionImplementor session, 
QueryParameters queryParameters, Boolean returnProxies)
at 
NHibernate.Loader.Loader.DoQueryAndInitializeNonLazyCollections(ISessionImplemen
tor session, QueryParameters queryParameters, Boolean returnProxies)
at NHibernate.Loader.Loader.DoList(ISessionImplementor session, QueryParameters 
queryParameters)

However, I believe one should not use one NHibernateDataContext instance in 
multiple threads as you do in the test. So it is ok for me that this test fails

I finally managed to build a test that reproduces my issue, however with a 
slightly different exception:

Tracker.Tests.ThreadSafeGetNamedQueryMultiContextTest:
System.AggregateException : One or more errors occurred.
  ----> System.NullReferenceException : Object reference not set to an instance of an object.
at System.Threading.Tasks.Task.WaitAll(Task[] tasks, Int32 millisecondsTimeout, 
CancellationToken cancellationToken)
at System.Threading.Tasks.Task.WaitAll(Task[] tasks, Int32 millisecondsTimeout)
at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
at Tracker.Tests.ThreadSafeGetNamedQueryMultiContextTest() in 
C:\Test\PLINQO-NH-Samples\CSharp\Tracker\Tests.cs:line 272
--NullReferenceException
at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, 
Boolean add)
at 
CodeSmith.Data.NHibernate.NHibernateDataContext.NHibernateDataContextAdvanced.ge
t_StateSession()
at 
CodeSmith.Data.NHibernate.NHibernateDataContext.NHibernateDataContextAdvanced.ge
t_DefaultSession()
at Tracker.Data.TrackerDataContext.GetUsersWithRoles() in 
C:\Test\PLINQO-NH-Samples\CSharp\Tracker\Data\TrackerDataContext.generated.cs:li
ne 212
at 
Tracker.Tests.<>c__DisplayClassf.<ThreadSafeGetNamedQueryMultiContextTest>b__e()
 in C:\Test\PLINQO-NH-Samples\CSharp\Tracker\Tests.cs:line 261
at System.Threading.Tasks.Task.Execute()

Finally, here the test:

        [Test]
        public void ThreadSafeGetSessionMultiContextTest()
        {
          Action action = () =>
            {
              using (var db = new TrackerDataContext())
              {
                var session = db.Advanced.DefaultSession; // may cause exception
                //var users = db.GetUsersWithRoles(); // this implicitly requires a DefaultSession
                //Assert.Greater(users.Count, 0);
              }
            };
          for (int tries = 0; tries < 10000; tries++)
          {
            var t1 = System.Threading.Tasks.Task.Factory.StartNew(action);
            var t2 = System.Threading.Tasks.Task.Factory.StartNew(action);
            System.Threading.Tasks.Task.WaitAll(t1, t2);
          }
        }

After I applied my fix this test passes.

Original comment by christia...@gmail.com on 27 Nov 2012 at 10:05

GoogleCodeExporter commented 9 years ago

Hello,

FYI: I updated the issue on google code with an answer to your post a while
ago without replying to this email. Its state is still WaitingInfo, maybe
you did not see my post. Therefore I write this email.

Original comment by christia...@gmail.com on 20 Dec 2012 at 10:10

GoogleCodeExporter commented 9 years ago

Thanks for this patch. I've updated this issue.

Original comment by bniemyjski on 20 Dec 2012 at 5:21

Changed state: Accepted
Added labels: Type-Patch
Removed labels: Type-Defect

GoogleCodeExporter commented 9 years ago

Hello,

This has been fixed in the latest nightly build. Thanks for the patch and 
reporting this issue.

On a side note, do you know why the ThreadSafeGetNamedQueryTest unit test is 
failing?

Original comment by bniemyjski on 17 Jan 2013 at 4:51

Changed state: Fixed

GoogleCodeExporter commented 9 years ago

Hello,

The ThreadSafeGetNamedQueryTest fails because multiple threads are using
the same DataContext, and therefore the same NHibernate session.
However, NHibernate sessions are not thread save as stated here:
http://nhibernate.hibernatingrhinos.com/12/nhibernate-and-the-unit-of-work-patte
rn-part-3
This article also explains how to implement a thread save unit of work
pattern.

Original comment by christia...@gmail.com on 28 Jan 2013 at 9:00

GoogleCodeExporter commented 9 years ago

Hello,

Thanks for that information. I'll update the test accordingly.

Original comment by bniemyjski on 1 Feb 2013 at 6:02

AnantLabs / codesmith

NHibernateDataContextAdvanced.StateSession not thread save #670