MarimerLLC / cslaforum

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

Collection was modified; enumeration operation may not execute. #213

Open alex4488 opened 8 years ago

alex4488 commented 8 years ago

Sometimes I get InvalidOperationException("Collection was modified; enumeration operation may not execute.") from DataPortal.Fetch(). The error seems to come from BusinessRules.HasPermission(AuthorizationActions, Type) when two threads call DataPortal.Fetch() at the same time. Here is a sample program to reproduce the error (you may need to adjust the number of rules and the delay):

`namespace CslaRuleTest { using System; using System.Threading; using Csla; using Csla.Rules;

public class Program
{
    public static void Main()
    {
        var thread = new Thread(Test);
        thread.Start();

        Thread.Sleep(1000);

        Test();
    }

    private static void Test()
    {
        try
        {
            TestEdit.GetTestEdit();
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex);
        }
    }
}

[Serializable]
public class TestEdit : BusinessBase<TestEdit>
{
    public static TestEdit GetTestEdit()
    {
        return DataPortal.Fetch<TestEdit>();
    }

    public static void AddObjectAuthorizationRules()
    {
        const int RuleCount = 30000;

        for (var i = 0; i < RuleCount; i++)
        {
            var action = (AuthorizationActions)(i + 10);

            BusinessRules.AddRule(typeof(TestEdit), new TestRule(action));
        }
    }

    private void DataPortal_Fetch()
    {
    }
}

public class TestRule : AuthorizationRule
{
    public TestRule(AuthorizationActions action)
        : base(action)
    {
    }

    protected override void Execute(AuthorizationContext context)
    {
        context.HasPermission = true;
    }
}

}`

Also, could you wrap the inner exception thrown from DataPortal.Fetch(Type, object) or use ExceptionDispatchInfo to rethrow it so that the original stacktrace is preserved.

rockfordlhotka commented 8 years ago

The short answer is that CSLA is not threadsafe, so if you have two threads trying to interact with the same business object graph at the same time I would expect failure.

Some parts of CSLA are theadsafe, in terms of clean isolation on the server side of the data portal, and multiple threads interacting with shared metastate (like business/authz rule definitions and property metastate definitions).

But business object instances are not threadsafe, just like normal .NET object instances aren't threadsafe unless you explicitly design them that way.

jonnybee commented 8 years ago

This blows up in BusinessRules.HasPermission as it is possible to get the

 AuthorizationRuleManager.GetRulesForType(objType, ruleSet).Rules

list while it is still being initialized in another tread. Tho' it is quite unlikely to happen as there can only be a mazimum of 4 Authz rules per RuleSet.

We could mitigate this by adding

  AuthorizationRuleManager.GetRulesForType(objType, ruleSet).Rules.ToArray()......

or modify the internal code so that we really block the type initialization when calling AddObjecAuthorizationRules.

alex4488 commented 8 years ago

@rockfordlhotka The error occurs in a static factory method so i'd expect it to be thread safe.

@jonnybee You are right, it's a rare error. We get this in a web service, maybe once in 100,000 calls, but it happened several times already so i'd like to fix it. I think that moving

mgr.InitializedPerType = true;

after invoking AddObjectAuthorizationRules would solve the problem. Something like this:

private static void InitializePerTypeRules(AuthorizationRuleManager mgr, Type type) { if (!mgr.InitializedPerType) lock (mgr) if (!mgr.InitializedPerType && !mgr.InitializingPerType) { // Only call AddObjectAuthorizationRules when there are no rules for this type if (RulesExistForType(type)) { mgr.InitializedPerType = true; return; }

    try
    {
      mgr.InitializingPerType = true;

      // invoke method to add auth roles
      const BindingFlags flags = BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy;
      System.Reflection.MethodInfo method = type.GetMethod("AddObjectAuthorizationRules", flags);
      if (method != null)
        method.Invoke(null, null);

      mgr.InitializedPerType = true;
    }
    catch (Exception)
    {
      // remove all loaded rules for this type
      CleanupRulesForType(type);
      throw;  // and rethrow the exception
    }
    finally
    {
      mgr.InitializingPerType = false;
    }
  }

}

jonnybee commented 8 years ago

Nope, that won't fix the issue. It will cause a "stack overflow" exception and ThreadAbort.

It is part of the solution but will require more refactoring to work.

alex4488 commented 8 years ago

@jonnybee I added a new property to AuthorizationRuleManager - InitializingPerType - to prevent stack overflow. I just tried the method from previous post and it worked fine.

alex4488 commented 8 years ago

Are there any plans to fix this issue? If not, is there any way to work around it?

rockfordlhotka commented 8 years ago

I'm on holiday this week and next, so I'm not doing much thinking about CSLA 😄

This does sound like a bug - the static initializers for types shouldn't fail. If you have a solution you can test it in your environment and either do a PR or post your fix here, otherwise when I'm back from vacation I'll add the issue to the backlog.

alex4488 commented 8 years ago

Thank you very much. Here is my solution (changed the method InitializePerTypeRules from Source/Csla.Shared/Rules/AuthorizationRuleManager.cs):

private bool InitializingPerType { get; set; }

private static void InitializePerTypeRules(AuthorizationRuleManager mgr, Type type)
{
  if (!mgr.InitializedPerType)
    lock (mgr)
      if (!mgr.InitializedPerType && !mgr.InitializingPerType)
      {
        // Only call AddObjectAuthorizationRules when there are no rules for this type
        if (RulesExistForType(type))
        {
          mgr.InitializedPerType = true;
          return;
        }

        try
        {
          mgr.InitializingPerType = true;

          // invoke method to add auth roles
          const BindingFlags flags = BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy;
          System.Reflection.MethodInfo method = type.GetMethod("AddObjectAuthorizationRules", flags);
          if (method != null)
            method.Invoke(null, null);
          mgr.InitializedPerType = true;
        }
        catch (Exception)
        {
          // remove all loaded rules for this type
          CleanupRulesForType(type);
          throw;  // and rethrow the exception
        }
        finally
        {
          mgr.InitializingPerType = false;
        }
      }
}

If you are going to fix this, could you also fix it in CSLA 4.5. We are still using the Silverlight version.