MarimerLLC / cslaforum

Discussion forum for CSLA .NET
https://cslanet.com
Other
31 stars 6 forks source link

Business rules not executing - possible threading issue #703

Open JacoJordaan opened 5 years ago

JacoJordaan commented 5 years ago

Question Is the following scenario suppose to work on a UI thread?

The idea is to have a WPF screen for a quote object and fetch/save or run async rules that may take some time and while this is busy the user can create/fetch another quote and work on this.

Currently this cause the second business object to not run any business rules while the first quote is still busy with its async operation.

I have the following test code to illustrate my issue: Simple business object with one validation rule that gets run on creation of the object. The save DataPortal_Insert() sleeps for 5 seconds to mimic a long running save.

[Serializable]
    public class BOEdit : BusinessBase<BOEdit>
    {
        #region Business Methods

        public static readonly PropertyInfo<int> StartValueProperty = RegisterProperty<int>(c => c.StartValue);
        public int StartValue
        {
            get { return GetProperty(StartValueProperty); }
            set { SetProperty(StartValueProperty, value); }
        }

        #endregion

        #region Business Rules

        protected override void AddBusinessRules()
        {
            BusinessRules.AddRule(new StarValueValidationRule(StartValueProperty));
        }

        #endregion

        #region Factory Methods

        public static BOEdit Create()
        {
            var data = DataPortal.Create<BOEdit>();

            data.BusinessRules.CheckRules();

            return data;
        }

        public BOEdit()
        { /* Require use of factory methods */ }

        #endregion

        #region Data Access

        [RunLocal]
        protected override void DataPortal_Create()
        {
            base.DataPortal_Create();
            using (BypassPropertyChecks)
            {
                StartValue = 2;
            }
        }

        protected override void DataPortal_Insert()
        {

            System.Threading.Thread.Sleep(5000);
            StartValue = 20;

        }

        #endregion
    }

The validation rule:

internal class StarValueValidationRule : CommonBusinessRule
    {
        internal StarValueValidationRule(IPropertyInfo primaryProperty)
            : base(primaryProperty)
        {
            if (InputProperties == null)
                InputProperties = new List<IPropertyInfo>();
            InputProperties.Add(primaryProperty);

            IsAsync = false;
            CanRunAsAffectedProperty = true;   // if false only run when this property is changed 
            CanRunInCheckRules = true;          // when true will also run in CheckRules
            CanRunOnServer = false;             // when true will also run on logical serverside (Data Access)
        }

        protected override void Execute(IRuleContext context)
        {
            var value = (int)context.InputPropertyValues[PrimaryProperty];

            if (value < 10)
                context.AddInformationResult("Expected start value to be greater than 10");

        }
    }

My test code:

           var bo = BOEdit.Create();

            Assert.AreEqual(2, bo.StartValue, "Expected result value = {0}", 2);
            Assert.AreEqual(1, bo.BrokenRulesCollection.Count, "Expected result value = {0}", 1);

            bo.BeginSave((o, e) =>
            {
                if (e.Error != null)
                    throw e.Error;
                else
                    bo = (BOEdit)e.NewObject;

                Assert.AreEqual(0, bo.BrokenRulesCollection.Count, "Expected result value = {0}", 0);
            });

            System.Threading.Thread.Sleep(1000); //Make sure the business object is busy saving

            var bo2 = BOEdit.Create();

            Assert.AreEqual(2, bo2.StartValue, "Expected result value = {0}", 2);
            Assert.AreEqual(1, bo2.BrokenRulesCollection.Count, "Expected result value = {0}", 1); //This fails with BrokenRulesCollection.Count = 0

My last line of test code is failing and the CheckRules does not enter the business rule code.

Version and Platform CSLA version: 4.7.200/4.9.0.0 OS: Windows Platform: WPF

jonnybee commented 5 years ago

There is a logic mistake in your code. Your BusinessRule is not allowed to run on the "logical" server side (ie: the rule will NOT run when BusinessRules.CheckRules is called in any DataPortal_XYZ methods nor when a property is changed in any DataPortal_XYZ method)

So you rule is ONLY allowed to run on the client side, ie your static Create method or when the property is changed on the client side.

Also, in you BOEdit - DataPortal_Create you are calling base.DataPortal_Create() first. This should be the last line in this method as all it does in the base class is:

[RunLocal]
protected virtual void DataPortal_Create()
{
  this.BusinessRules.CheckRules();
}

And using(BypassPropertyChecks) means do not run rules as properties is set as it is expected to call CheckRules in last line.

In your DataPortal_Insert method you set the property value to 20, which is a legel value. However - the rule engine is clearing BrokenRulesCollection for all "broken rules" that originated from that property and then reruns the rules. In your code the rule is not allowed to run = the field is now valid and there is no entries in BrokenRulesCollection.

JacoJordaan commented 5 years ago

Hi Jonny,

Appreciate your feedback.

Please note that I am calling CheckRules in my static Create method after the server side code returns in order to run the rules. My tests show that this is working.

My intention was actually not to run the rules in the DataPortal_Create method. I have taken out the base.DataPortal_Create(); statement.

The code to set the value to 20 was for other testing purposes. I have taken this out as well to avoid confusion. So, the broken rules count should always return 1. My modified code now looks like this:


        public BOEdit()
        { /* Require use of factory methods */ }

        #region Data Access

        [RunLocal]
        protected override void DataPortal_Create()
        {            
            using (BypassPropertyChecks)
            {
                StartValue = 2;
            }
        }

        protected override void DataPortal_Insert()
        {

            System.Threading.Thread.Sleep(5000);            

        }

            #endregion

My tests still fails on last line. When I am running the tests with the sync save like this then all is working:

            var bo = BOEdit.Create();

            Assert.AreEqual(1, bo.BrokenRulesCollection.Count, "Expected result value = {0}", 1);

            bo = bo.Save();
            Assert.AreEqual(1, bo.BrokenRulesCollection.Count, "Expected result value = {0}", 1);

            var bo2 = BOEdit.Create();

            Assert.AreEqual(1, bo2.BrokenRulesCollection.Count, "Expected result value = {0}", 1); 

The tests with async save still fails on last line - the second business object does not fire the business rules when CheckRules is called:


            var bo = BOEdit.Create();

            Assert.AreEqual(1, bo.BrokenRulesCollection.Count, "Expected result value = {0}", 1);

            bo.BeginSave((o, e) =>
            {
                if (e.Error != null)
                    throw e.Error;
                else
                    bo = (BOEdit)e.NewObject;

                Assert.AreEqual(1, bo.BrokenRulesCollection.Count, "Expected result value = {0}", 1);
            });

            System.Threading.Thread.Sleep(1000); //Make sure the business object is busy saving

            var bo2 = BOEdit.Create();

            Assert.AreEqual(1, bo2.BrokenRulesCollection.Count, "Expected result value = {0}", 1); //This fails with BrokenRulesCollection.Count = 0

Am I missing something?

jonnybee commented 5 years ago

The issue here is that when using the LocalProxy the value of ApplicationContext.LogicalExecutionContext is Server until the BeginSave has completed.

So when you call the second BOEdit.Create the ApplicationContext is still set for Server and hence the validation rule is not allowed to run.

@rockfordlhotka Is this a bug or it is intentional?

rockfordlhotka commented 5 years ago

It is sort of intentional, in that this is very likely all running on a single thread, and there's no way to know the logical location of some arbitrary code running on this single thread. We could force the server-side code to run on a different thread, but that has a cost to it, and the theory is that people are using the local proxy to get the best performance.

If you want to have the server-side code running on a different thread, you can create your own proxy/host channel - there's an example in the test project iirc.

JacoJordaan commented 5 years ago

Appreciate all the feedback.

@rockfordlhotka, if I take out the [RunLocal] attribute and force the create to run on the server even though it is not doing anything, will this work? Or am I on the wrong track here.

jonnybee commented 5 years ago

This may actually be a bug.

The Csla.Core.4.9.0 package and the net461\Csla.dll is NOT compiled with NET40 or NET45 compiler directive and hence the LocalContext is stored in a member variable rather than NamedDataSlot in current thread in ApplicationContextManager.

@rockfordlhotka Is this intentional?

jonnybee commented 5 years ago

Decompiled code from \net461\Csla.dll:

   /// <summary>
    /// Default context manager for the user property
    /// and local/client/global context dictionaries.
    /// </summary>
    public class ApplicationContextManager : IContextManager
    {
        private AsyncLocal<ContextDictionary> _localContext = new AsyncLocal<ContextDictionary>();
        private AsyncLocal<ContextDictionary> _clientContext = new AsyncLocal<ContextDictionary>();
        private const string _globalContextName = "Csla.GlobalContext";

Decompiled code from \net45\csla.dll:

    /// <summary>
    /// Default context manager for the user property
    /// and local/client/global context dictionaries.
    /// </summary>
    public class ApplicationContextManager : IContextManager
    {
        private const string _localContextName = "Csla.LocalContext";
        private const string _clientContextName = "Csla.ClientContext";
        private const string _globalContextName = "Csla.GlobalContext";

This seems like a bug to me!

JacoJordaan commented 5 years ago

Thanks @jonnybee ,

It will makes sense if it is a bug. I am experiencing similar issues with async lazy loading of command object in a property get causing create/fetch of another object not calling the business rules while the async loading is running. I have not confirmed this yet with a test.

rockfordlhotka commented 5 years ago

It is a known breaking change. Which doesn't mean it isn't a bug, but it is intentional.

https://github.com/MarimerLLC/csla/issues/729

The change to AsyncLocal was done to allow context to flow through async call stacks based on Microsoft's recommended solution. This change makes for consistent behavior across scenarios that were inconsistent in the past.

Maybe we can improve the change in some ways, but I don't want to completely throw out AsyncLocal for context management, because I think there's a net benefit to being in-line with Microsoft's direction.