New async validation rules - Possible Problem???

vdhant posted on Monday, November 24, 2008

I have been playing around a little with the async validation rules and have noticed a little bit of a problem.

If "void CheckRules(List<IRuleMethod> list)" throws an exception, in the catch statement it calls "rule.RuleArgs.PropertyName". Before the async validation rules came along this would have been fine. But in the case where async validation rule throws an exception, AsyncRuleMethod throws an exception when RuleArgs is called.

    catch (Exception ex)
    {
        //Throw a more detailed exception
        throw new ValidationException(string.Format(Resources.ValidationRulesException, rule.RuleArgs.PropertyName, rule.RuleName), ex);
    }

internal class AsyncRuleMethod ... {
....
        RuleArgs IRuleMethod.RuleArgs
        {
            get { throw new NotSupportedException(); }
        }
....
}

cheers
Anthony

RockfordLhotka replied on Monday, November 24, 2008

I don't believe this is a bug. This is by design.

An async rule method can never allow an unhandled exception to occur. Ever.

Async rule methods must always catch all exceptions and must return them through the parameter provided in the callback.

This is because an async rule method is running on a background thread (or may be) and so an unhandled exception would then flow up to the .NET runtime as an unhandled exception - and that's a problem.

So we have a scheme by which an exception in the async rule method can be safely returned to CSLA, which can then take care of it on the primary thread. But never in the CheckRules() method - because that merely starts the async rule and never handles any result.

Of course you may fully understand this already - in which case I'm misunderstanding you, and I need more clarification.

vdhant replied on Tuesday, November 25, 2008

Hi Rocky thanks for the reply...
I wasn't pointing out the need to catch the exception and then re-throw it, i totally get that.

I was trying to point out the face that the "rule.RuleArgs.PropertyName" part in the catch statement causes a NotSupportedException to occur when using a rule that caused the error is of type AsyncRuleMethod (which is the case when dealing with Async rules).

In other words given the catch statements in CheckRules, I would expect that this catch statement be run if an error occurs whilst processing the validation rule. Next I would expect that the catch statement would throw a new exception of type ValidationException which contains whatever the original exception was. In the case where the rule that fails is of type RuleMethod, this works fine....

But In the case where rule of type AsyncRuleMethod when following code is executed "throw new ValidationException(string.Format(Resources.ValidationRulesException, rule.RuleArgs.PropertyName, rule.RuleName), ex);" the bolded part thow a NotSupportedException excecption which contains no information about the original exception. The reason why this occurs is because AsyncRuleMethod exsplicity implements the RuleArgs property and throws a NotSupportedException exception if it is ever called. When this occurs the application gets back a NotSupportedException exception which has nothing to do with the error that occurred in the first place and not what of finding out how the original exception occurred.

I hope that makes a bit more sense.
Cheers
Anthony

RockfordLhotka replied on Tuesday, November 25, 2008

I am still not sure there’s an issue.

CheckRules() doesn’t execute async rules. It merely starts the rule. The rule method can’t fail – async rule methods must never throw an exception – so that catch block should never be hit.

Rocky

vdhant replied on Tuesday, November 25, 2008

Hi Rocky
You have just said that the CheckRules method doesn't execute async rules and that it only states/finds the async rules... I'm sorry but I may be way off the mark but I thought that the following section of code was executing any found async rules within the CheckRules method (note the bolded invoke)...

    private void CheckRules(List<IRuleMethod> list)
    {
    ....

        IAsyncRuleMethod[] asyncRules = null;
        lock (SyncRoot)
            asyncRules = ValidatingRules.ToArray();

        // They must all be added to the ValidatingRules list before you can invoke them.
        // Otherwise you have a race condition, where if a rule completes before the next one is invoked
        // then you may have the ValidationComplete event fire multiple times invalidly.
        foreach (IAsyncRuleMethod rule in asyncRules)
        {
            try
            {
                rule.Invoke(_target, asyncRule_Complete);
            }
            catch (Exception ex)
            {
                // throw a more detailed exception
                throw new ValidationException(
                    string.Format(Properties.Resources.ValidationRulesException, rule.RuleArgs.PropertyName,
                                  rule.RuleName), ex);
            }
        }
    }

Also please note that the problem that I have been trying to raise is about the catch statement that surrounds this invoke, the other catch statement within CheckRules should be fine as you have pointed out, but I can't see how this one can be ok.

If any errors occur within method that is invoked (and doesn't happen on a different thread... i.e. the rule that has been executed hasn't spun up a new thread yet), the problem that I have described will occur.

Hope this helps and for the record I may have total missed something that is going on here so just let me know if I’m off the mark and I will leave it, but at the moment I can't how the second catch statement can't be an issue, because it exists only to catch exception generated by the invoke of the rules that are of type async.

Cheers
Anthony

RockfordLhotka replied on Wednesday, November 26, 2008

It is a bit confusing when you look at the code.

But the rule is very strict: an async rule method may never throw an exception.

So yes, CheckRules() “runs” the async method. But really the idea is that the async method does its work on a background thread. It shouldn’t do anything on the calling thread, so there really shouldn’t be anything to actually fail back into CheckRules(). Any failure should occur in the rule processing on the background thread, and that exception should be returned through the callback.

If you somehow do create an async method that does work on the calling thread that could fail, there are two things to consider. First, it isn’t async yet, so this is synchronous code. Second, it should still never allow an unhandled exception (see the rule), and so any failure should cause the exception to be returned via the callback.

The whole async rule infrastructure is based on the rule that an async method may never throw an exception. That any exception, in any part of the rule, is returned via the callback. That every rule has a callback, whether it failed or not.

This affects exception handling, but also the IsBusy property and related processing.

Rocky

vdhant replied on Wednesday, November 26, 2008

Cool thanks for that... I only have one question then, why have the try catch block around the async code, if 1) its never going to be used and 2) if it does get called somehow, the catch block fails because of the problem that I have described? I would think that if you are going to have a try/catch block the last thing you would want is the catch to throw a new exception as its throwing exception, regardless of whether it "should" be called. My problem is not whether the try/catch should or shouldn't be there but the fact that if it is going to be there, but the fact that the catch is not going to work as intended if it happened to run.
Cheers
Anthony

RockfordLhotka replied on Wednesday, November 26, 2008

I think you are right in that in that regard it is a bug – and I’ll put it on the list.

Thanks!!

Rocky

From: vdhant [mailto:cslanet@lhotka.net]
Sent: Wednesday, November 26, 2008 1:57 PM
To: rocky@lhotka.net
Subject: Re: [CSLA .NET] RE: RE: New async validation rules - Possible Problem???

RockfordLhotka replied on Wednesday, December 03, 2008

My proposed solution to this problem is to have AsyncRuleMethod.RuleArgs return a valid value

RuleArgs IRuleMethod.RuleArgs
{
get { return new RuleArgs(_args.Properties[0].Name); }
}

This shouldn't really happen (as discussed earlier in this thread), but if someone does mess up and allow an unhandled exception to flow out of an async rule method, at least the result is predictable.

Does that seem reasonable to you?