Async After-/BeforeTransactionCompletion #1452

gliljas · 2017-11-23T22:27:29Z

While working on #1392 I recognized that the AfterTransactionComplete will always be executed synchronously, even if the rest is async. The only solution I could think of was to do what I've done in this PR. The typed delegates were replaced with interfaces, as it is in Hibernate. The reason for using delegates was clearly to allow for inline definitions of the "process" (cryptically described in the code comments). I made two "wrapper" classes for this, which takes an Action and a Func to handle such inlining.

I'm not quite sure how to configure AsyncGenerator to do the right things.

Yes, this is a breaking change :)

Update by Frédéric: it is no more breaking thanks to Alexander changes.

gliljas · 2017-11-23T22:47:58Z

Nested partials could be an option too, i.e. create small nested classes, which can access the containing type's members.

fredericDelaporte

Maybe @maca88 would have a better idea than my comments below.

fredericDelaporte · 2017-11-24T10:50:39Z

src/NHibernate/Action/BulkOperationCleanupAction.cs

+					(success) =>
+					{
+						EvictEntityRegions();
+						EvictCollectionRegions();


If the sync delegate is defined as a method, the Async Generator should then be able to generate its async counter-part, which could then be referenced as the async delegate here.

So from dev perspective, this is a three steps operation: define the sync method and use it as the sync delegate (along with a dummy async one or by using the sync only constructor), generate async, use the async counterpart as the async delegate.

Same for other places where there is those delegates to supply.

It's in a backlog maca88/AsyncGenerator#70

(along with a dummy async one or by using the sync only constructor)

Another way is to directly use the constructor that has both sync and async actions and use the async method even if it is not generated yet, this way you can save one code generation.

I've made an example for this PR, how the code could look like prior the generation of the async code.

@hazzik For this PR, the mentioned generator feature wouldn't help, as here we need both sync and async methods to be passed in the constructor method, similar to what we have for Future.

fredericDelaporte · 2017-11-24T10:51:38Z

src/NHibernate/Action/CollectionAction.cs

-						persister.Cache.Release(ck, softLock);
-					});
+					return new AfterTransactionCompletionProcess(
+						(success) =>


Same as previous comment.

fredericDelaporte · 2017-11-24T10:51:45Z

src/NHibernate/Action/CollectionUpdateAction.cs

-
-						if (success)
+					return new AfterTransactionCompletionProcess(
+						(success) =>


Same as previous comment.

gliljas · 2018-01-24T10:13:37Z

@hazzik @fredericDelaporte

What are our thoughts on what constitutes a breaking change? The IExecutable interface is public, but...

Apart from BulkOperationCleanupAction all implementations are sealed
Actions are explicitly typed in all ActionQueue methods.
Implementing IExecutable is basically useless

fredericDelaporte · 2018-01-24T10:31:34Z

It is still a binary breaking change, so normally not acceptable in a minor release when following SemVer. Is it really inconvenient to follow the pattern seen in some other "would be interface change" cases, where it is has been dodged with a combination of extension methods and cast to known concrete implementation?

hazzik · 2018-08-22T12:23:20Z

@fredericDelaporte @gliljas I made some refactoring, please review. I'm going to remove breaking changes later so it could be included into 5+

fredericDelaporte

LGTM

…mpletion

fredericDelaporte · 2018-08-23T13:02:14Z

src/NHibernate/Action/IAsyncExecutable.cs

@@ -0,0 +1,15 @@
+namespace NHibernate.Action
+{
+	public interface IAsyncExecutable : IExecutable


I would add a // 6.0 TODO: merge into IExecutable. Or is there any value in keeping a separated interface for async methods?

fredericDelaporte · 2018-08-23T13:04:17Z

src/NHibernate/Action/IExecutable.cs

+	/// place of the Hibernate interface (see Action/AfterTransactionCompletionProcess). The
+	/// delegate omits the <see cref="ISessionImplementor" /> parameter as it is not used.
+	/// </remarks>
+	public delegate void AfterTransactionCompletionProcessDelegate(bool success);


Shouldn't these delegates be obsoleted?

fredericDelaporte · 2018-08-23T13:05:18Z

src/NHibernate/Action/IExecutable.cs


 		/// <summary>
 		/// Get the after-transaction-completion process, if any, for this action.
 		/// </summary>
-		IAfterTransactionCompletionProcess AfterTransactionCompletionProcess { get; }
+		AfterTransactionCompletionProcessDelegate AfterTransactionCompletionProcess { get; }


I would obsolete these properties.

fredericDelaporte · 2018-08-23T13:06:51Z

src/NHibernate/Engine/ActionQueue.cs

 		}
-
+
+		public void RegisterProcess(AfterTransactionCompletionProcessDelegate process)


I would obsolete these methods.

fredericDelaporte · 2018-08-23T13:10:22Z

src/NHibernate/Engine/ActionQueue.cs

+			if (executable is IAsyncExecutable asyncExecutable)
+			{
+				beforeTransactionProcesses.Register(asyncExecutable.BeforeTransactionCompletionProcess);
+				afterTransactionProcesses.Register(asyncExecutable.AfterTransactionCompletionProcess);


It could be:

RegisterProcess(asyncExecutable.BeforeTransactionCompletionProcess); RegisterProcess(asyncExecutable.AfterTransactionCompletionProcess);

Slightly less code written, and the compiler will likely inline the call anyway.

fredericDelaporte · 2018-08-23T13:11:15Z

src/NHibernate/Engine/ActionQueue.cs

+			}
+		}
+
+		private partial class AfterTransactionCompletionDelegatedProcess : IAfterTransactionCompletionProcess


I would obsolete these classes too.

fredericDelaporte · 2018-09-14T10:05:33Z

This seems all good. Any reason for not merging it yet?

fredericDelaporte reviewed Nov 24, 2017

View reviewed changes

fredericDelaporte added this to the 6.0 milestone Nov 24, 2017

gliljas force-pushed the asyncexecutable branch from 651fdfe to 41342c3 Compare January 26, 2018 21:15

hazzik mentioned this pull request Jun 15, 2018

Async ISynchronization #1752

Closed

hazzik mentioned this pull request Aug 22, 2018

using IPostInsertEventListener - OnPostInsertAsync not called #1826

Closed

Replaced TransactionCompletion delegates with interface implementations

a592363

hazzik force-pushed the asyncexecutable branch 4 times, most recently from e41f740 to 8666d48 Compare August 22, 2018 11:23

Refactor IBefore/AfterTransactionCompletionProcess to avoid delegates

8b22484

hazzik force-pushed the asyncexecutable branch from 8666d48 to 8b22484 Compare August 22, 2018 12:03

hazzik added t: Improvement p: Major labels Aug 22, 2018

fredericDelaporte reviewed Aug 23, 2018

View reviewed changes

hazzik added 2 commits August 23, 2018 22:35

Simplify call of BulkOperationCleanupAction.ExecuteAfterTransactionCo…

b61a907

…mpletion

Remove breaking changes

e26f335

hazzik force-pushed the asyncexecutable branch from c07b41f to e26f335 Compare August 23, 2018 12:48

fredericDelaporte reviewed Aug 23, 2018

View reviewed changes

Obsole Before-/AfterTransactionCompletionProcessDelegate

f2b0cd9

hazzik modified the milestones: 6.0, 5.2 Aug 24, 2018

fredericDelaporte approved these changes Aug 24, 2018

View reviewed changes

fredericDelaporte added the c: Core label Aug 27, 2018

fredericDelaporte mentioned this pull request Sep 16, 2018

Allow to use dynamic objects as dynamic components #1778

Merged

hazzik changed the title ~~Async After-/BeforeTransactionComplete~~ Async After-/BeforeTransactionCompletion Sep 16, 2018

hazzik merged commit ea4c7ff into nhibernate:master Sep 16, 2018

hazzik added the r: Fixed label Sep 16, 2018

fredericDelaporte mentioned this pull request Sep 19, 2018

Avoid initializing proxies when ordering inserts #1852

Merged

gliljas deleted the asyncexecutable branch October 4, 2018 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async After-/BeforeTransactionCompletion #1452

Async After-/BeforeTransactionCompletion #1452

gliljas commented Nov 23, 2017 •

edited by fredericDelaporte

Loading

gliljas commented Nov 23, 2017

fredericDelaporte left a comment

fredericDelaporte Nov 24, 2017

hazzik Nov 24, 2017

maca88 Nov 24, 2017

fredericDelaporte Nov 24, 2017

fredericDelaporte Nov 24, 2017

gliljas commented Jan 24, 2018

fredericDelaporte commented Jan 24, 2018 •

edited

Loading

hazzik commented Aug 22, 2018

fredericDelaporte left a comment

fredericDelaporte Aug 23, 2018

fredericDelaporte Aug 23, 2018

fredericDelaporte Aug 23, 2018

fredericDelaporte Aug 23, 2018

fredericDelaporte Aug 23, 2018

fredericDelaporte Aug 23, 2018

fredericDelaporte commented Sep 14, 2018 •

edited

Loading

		}


		public void RegisterProcess(AfterTransactionCompletionProcessDelegate process)

Async After-/BeforeTransactionCompletion #1452

Async After-/BeforeTransactionCompletion #1452

Conversation

gliljas commented Nov 23, 2017 • edited by fredericDelaporte Loading

gliljas commented Nov 23, 2017

fredericDelaporte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gliljas commented Jan 24, 2018

fredericDelaporte commented Jan 24, 2018 • edited Loading

hazzik commented Aug 22, 2018

fredericDelaporte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Sep 14, 2018 • edited Loading

gliljas commented Nov 23, 2017 •

edited by fredericDelaporte

Loading

fredericDelaporte commented Jan 24, 2018 •

edited

Loading

fredericDelaporte commented Sep 14, 2018 •

edited

Loading