NH-3606 - open a stateless session from a session #1520

fredericDelaporte · 2018-01-10T15:24:22Z

fredericDelaporte · 2018-01-10T15:35:55Z

src/NHibernate.Test/TransactionTest/TransactionFixture.cs

@@ -173,6 +173,35 @@ public void FlushFromTransactionAppliesToSharingSession()
 			}
 		}

+		[Test]
+		public void FlushFromTransactionAppliesToSharingStatelessSession()


I have not added the equivalent test in system transaction, because the "flush from transaction completion" feature is basically broken with stateless sessions. A stateless session accumulates changes as a pending batched command, bound to the current connection. When flushing from a system transaction, this means the command has to execute from the NHibernate enlisted resource prepare phase, potentially after (or concurrently to) its connection own prepare phase.

hazzik · 2018-01-22T11:05:14Z

Maybe it's better to move duplicated code into AbstractSessionImpl?

fredericDelaporte · 2018-01-22T12:18:41Z

I have to check how impactful it would be. Especially if this results in moving connectionManager to the base class.

fredericDelaporte · 2018-01-22T16:38:11Z

Maybe it's better to move duplicated code into AbstractSessionImpl?

Done. This causes less changes than what I was expecting. I have still done it in multiple commits in order to allow dropping the last ones if preferring less changes.

fredericDelaporte · 2018-01-22T16:41:48Z

src/NHibernate/Engine/ISessionImplementor.cs

+		/// A session is considered connected if there is a <see cref="DbConnection"/> (regardless
+		/// of its state) or if the field <c>connect</c> is true. Meaning that it will connect
+		/// at the next operation that requires a connection.
+		/// </remarks>


This property concrete implementation being put in common, I have taken the more complete xml documentation.

fredericDelaporte · 2018-01-22T16:43:20Z

src/NHibernate/Impl/AbstractSessionImpl.cs

 		public ITransactionContext TransactionContext
 		{
 			get; set;
 		}

 		private bool isAlreadyDisposed;

-		private static readonly INHibernateLogger logger = NHibernateLogger.For(typeof(AbstractSessionImpl));
+		private static readonly INHibernateLogger Log = NHibernateLogger.For(typeof(AbstractSessionImpl));


It was unused. With the code moves, it is now used, and renamed by the way.

fredericDelaporte · 2018-01-22T16:45:11Z

src/NHibernate/Impl/AbstractSessionImpl.cs

+						this,
+						options.UserSuppliedConnection,
+						options.SessionConnectionReleaseMode,
+						Interceptor,


Overridden in stateless for always returning EmptyInterceptor, so equivalent to previous code in stateless, which was creating the connection manager with EmptyInterceptor.

fredericDelaporte · 2018-01-22T16:45:47Z

src/NHibernate/Impl/AbstractSessionImpl.cs

+					ConnectionManager = new ConnectionManager(
+						this,
+						options.UserSuppliedConnection,
+						options.SessionConnectionReleaseMode,


In stateless option, always return AfterTransaction, so equivalent to previous code in stateless.

fredericDelaporte · 2018-01-22T16:47:31Z

src/NHibernate/Impl/AbstractSessionImpl.cs

-		public abstract IBatcher Batcher { get; }
+		// 6.0 TODO: remove virtual.
+		/// <inheritdoc />
+		public virtual IBatcher Batcher


Since it was abstract, and since AbstractSessionImpl was public, it would be a breaking change to not allow it to be overridden. But there are nor reason to keep it virtual, thus the TODO for 6.0. The same applies to a bunch of other properties later in this file.

fredericDelaporte · 2018-01-22T16:51:27Z

src/NHibernate/Impl/AbstractSessionImpl.cs

-		public abstract bool TransactionInProgress { get; }
+		// 6.0 TODO: remove virtual.
+		/// <inheritdoc />
+		public virtual bool TransactionInProgress => ConnectionManager.IsInActiveTransaction;


In stateless, this was Transaction.IsActive instead. It looks to me as an undue discrepancy. I have removed this discrepancy. But it was not having much impact, since this is an unused ISessionImplementor property. Its documentation backs my change further:

/// <summary> /// Does this <c>ISession</c> have an active NHibernate transaction /// or is there a system transaction in progress in which the session is enlisted? /// </summary>

fredericDelaporte · 2018-01-22T16:54:30Z

src/NHibernate/Impl/AbstractSessionImpl.cs

@@ -218,19 +256,24 @@ public virtual IQuery GetNamedSQLQuery(string name)
 			}
 		}

+		// 6.0 TODO: remove virtual from below properties.
+		/// <inheritdoc />
+		public virtual ConnectionManager ConnectionManager { get; protected set; }


Serialization of SessionImpl constrains to have the setter. Otherwise we should explicitly implement serialization in this base class too. (Which theoretically would imply explicitly implementing it in stateless too, but stateless serialization seems to me already broken.)

Changing this to an automatic property makes it serializable while it was marked as [NonSerialized] before (in StatelessSession).

If we were willing to fix serialization for stateless, we would need to serialize it. Not having it serializable is a bug. Since we need to serialize it for SessionImpl, better reflect that fact. This anyway changes nothing for stateless, since it cannot be deserialized to an usable object currently.

fredericDelaporte · 2018-01-22T16:57:48Z

src/NHibernate/Impl/StatelessSessionImpl.cs

@@ -28,9 +25,6 @@ public partial class StatelessSessionImpl : AbstractSessionImpl, IStatelessSessi
 	{
 		private static readonly INHibernateLogger log = NHibernateLogger.For(typeof(StatelessSessionImpl));

-		[NonSerialized]
-		private readonly ConnectionManager connectionManager;


A deserialized session without a connection manager is quite useless. I guess no-one serializes stateless sessions, or maybe we have a bug logged somewhere about it. ~~Anyway, it does not even have a default constructor or a deserialization constructor, so it can only fail deserializing.~~

Yes, we have a bug.

hazzik · 2018-01-22T20:53:06Z

I was thinking about this myself, and made a conclusion that StatelessSession is (was always) not serializable. Eg, the connectionManager property would always be null after deserialization.

hazzik · 2018-01-22T20:54:44Z

src/NHibernate/Impl/AbstractSessionImpl.cs

@@ -218,19 +256,24 @@ public virtual IQuery GetNamedSQLQuery(string name)
 			}
 		}

+		// 6.0 TODO: remove virtual from below properties.
+		/// <inheritdoc />
+		public virtual ConnectionManager ConnectionManager { get; protected set; }


Changing this to an automatic property makes it serializable while it was marked as [NonSerialized] before (in StatelessSession).

hazzik · 2018-01-22T20:56:50Z

src/NHibernate/Impl/AbstractSessionImpl.cs

+		/// <summary>Get the current NHibernate transaction.</summary>
+		public ITransaction Transaction => ConnectionManager.Transaction;
+
+		protected bool TransactionCoordinatorShared { get; }


In other places it's called "IsTransactionCoordinatorShared". Changing it to an automatic property makes the field serializable, while it was marked as [NonSerialized] before.

Yes, but no, and without any importance currently, that is why I have not really bothered.

For SessionImpl, serialization is explicitly done, causing fields from the base class to be serialized only if handled explicitly. It is not in SessionImpl. Anyway, if it was serialized, it would not change anything, because GetObjectData throws if it is not false, so a value other than the default one for its type would not be serialized.

For stateless, well, it could be now serialized, if the stateless was actually serializable, which is still not I believe. Stateless should implement serialization the same way than SessionImpl, meaning choosing itself what it serialize and if it can be serialized.

Update: I do not think that we should switch away from an automatic property just for being able to signal it is not serialized, while its serialization or non-serialization cannot have any impact currently.

hazzik · 2018-01-22T20:57:56Z

src/NHibernate/Impl/StatelessSessionImpl.cs

@@ -28,9 +25,6 @@ public partial class StatelessSessionImpl : AbstractSessionImpl, IStatelessSessi
 	{
 		private static readonly INHibernateLogger log = NHibernateLogger.For(typeof(StatelessSessionImpl));

-		[NonSerialized]
-		private readonly ConnectionManager connectionManager;


Yes, we have a bug.

- Fixes nhibernate#910

This is not fixing internal troubles since this property has no usages.

Just to get rid of the warning sign (at least with my current settings...).

fredericDelaporte · 2018-02-28T12:22:00Z

Rebased.

fredericDelaporte added the t: Fix label Jan 10, 2018

fredericDelaporte added this to the 5.1 milestone Jan 10, 2018

fredericDelaporte mentioned this pull request Jan 10, 2018

NH-3606 - A method to create a child stateless session #314

Closed

fredericDelaporte commented Jan 10, 2018

View reviewed changes

fredericDelaporte force-pushed the NH-3606 branch from d94b00b to 3f765c4 Compare January 22, 2018 16:36

fredericDelaporte commented Jan 22, 2018

View reviewed changes

hazzik reviewed Jan 22, 2018

View reviewed changes

fredericDelaporte mentioned this pull request Jan 23, 2018

Stateless session is not serializable #1553

Open

fredericDelaporte force-pushed the NH-3606 branch from bf00ee8 to 6315627 Compare January 23, 2018 10:13

fredericDelaporte added 8 commits February 28, 2018 14:20

NH-3606 - open a stateless session from a session

6ac5fa0

- Fixes nhibernate#910

Minimal move of duplicated code.

875f28d

Additional move of duplicated code.

51b6ee8

Move in common overrides duplicated since connection manager own move.

e11c333

Move in common a property duplicated since connection manager own move.

3e6da46

Move in common a property with an undue discrepancy.

683f3af

This is not fixing internal troubles since this property has no usages.

Trivial additional clean-up.

cc2d431

Just to get rid of the warning sign (at least with my current settings...).

Rename a property for consistency.

30d8fe2

fredericDelaporte force-pushed the NH-3606 branch from 6315627 to 30d8fe2 Compare February 28, 2018 12:21

Merge branch 'master' into NH-3606

7e468e8

hazzik approved these changes Mar 11, 2018

View reviewed changes

fredericDelaporte merged commit dce4d02 into nhibernate:master Mar 11, 2018

fredericDelaporte deleted the NH-3606 branch March 11, 2018 23:38

fredericDelaporte added the r: Fixed label Mar 11, 2018

NH-3606 - open a stateless session from a session #1520

NH-3606 - open a stateless session from a session #1520

Uh oh!

Conversation

fredericDelaporte commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Jan 22, 2018

Uh oh!

fredericDelaporte commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Feb 28, 2018

Uh oh!

Uh oh!

fredericDelaporte commented Jan 22, 2018 •

edited

Loading

fredericDelaporte Jan 23, 2018 •

edited

Loading

fredericDelaporte Jan 22, 2018 •

edited

Loading

fredericDelaporte Jan 23, 2018 •

edited

Loading