jwaliszko / ExpressiveAnnotations

Annotation-based conditional validation library.
MIT License
351 stars 123 forks source link

ValidationException in multi-threaded environment #34

Closed pawelpabich closed 9 years ago

pawelpabich commented 9 years ago

Hi,

We have a set of e2e Selenium based tests that we run in parallel on our build server. Since we started using ExpressiveAnnotations we can see every now and then the following error which we've never seen on our local dev machines where we run tests only sequentially. To be honest I have no idea where to start re debugging this problem. Any hints would be appreciated.

View model:

        [Required]
        [DisplayName("Has Deck Gear?")]
        public bool DeckGear { get; set; }

        [DisplayName("Number of Deck Gear")]
        [Validation.RequiredIf("DeckGear", ErrorMessage = "Required if ship has a deck gear")]
        public int? NumberOfDeckGear { get; set; }

Error:

image

jwaliszko commented 9 years ago

Hi,

You could start to debug through this issue starting from line 45 of RequiredIfValidator.cs:

image

Notice that because of the optimization purposes, this line is invoked only once during the startup of the application - then the compiled lambda is cached and expression is never parsed again. Due to that, to be able to debug through this line on each request, you may need to set the FieldsMap and the ConstsMap variables to null, for the time of debugging:

image

The stack trace shows that exception variable '' of type 'System.Object' referenced from scope '', but it is not define is thrown for some reason by Expression Trees library from inside Parser.cs. I know this is not much help but I've never used this library with Selenium, so it is hard for me to guess what could go wrong. I can only assume that maybe the metadata.ContainerType which supposed to be of your model type is something different or maybe there are some issues with the reflection. It definitely needs some attention though.

pawelpabich commented 9 years ago

Thanks for that but the problem manifests itself when we run multiple tests in parallel so I can't really debug it manually. I don't think selenium is the important part here as it simply drives web browsers that hit my website. I have no evidence but this feels to me like a race condition. Like a missing lock statement maybe? Does RequiredIfAttribute needs to be thread safe?

jwaliszko commented 9 years ago

We can give it a try. I've synchronized access to the core methods of parser and tokenizer (I'll send you modified and compiled .NET45 DLL in a moment):

public Func<Context, bool> Parse<Context>(string expression)
{
    lock (_locker)
    {

public IEnumerable<Token> Analyze(string expression)
{
    lock (_locker)
    {

Nevertheless, if it works it is a surprise for me because each attribute (as well as each validator) has its own parser instance so assuming the library is used in a standard way it should not face multithread-related issues.

pawelpabich commented 9 years ago

I pushed one more change before I got your dll. Initially

 DataAnnotationsModelValidatorProvider.RegisterAdapter(typeof(RequiredIfAttribute), typeof(RequiredIfValidator));
            DataAnnotationsModelValidatorProvider.RegisterAdapter(typeof(AssertThatAttribute), typeof(AssertThatValidator));

were not the very first 2 lines in Application_Start. Now they are and we will see how it goes. If I see this problem again. I will use the dlls you sent me.

Thanks a lot for help. I will report back my findings.

pawelpabich commented 9 years ago

Ok. The last change did not fix the problem. I have seen it again. I will use your dll.

jwaliszko commented 9 years ago

I'm quite interested if it worked or not.

If the problem is still there, I've pushed few more synchronization-related changes to the repository - so (assuming the DLL I've sent you yesterday hasn't solved this issue) you could get latest sources, rebuild them in debug mode, and check again.

If even after that the problem is still there, it will mean this exception is (almost certainly) not related to multithreading, and we'll try to search for fix somewhere else.

pawelpabich commented 9 years ago

Cool

pawelpabich commented 9 years ago

Hi,

So far so good. Since we moved to the new version with locks we have not seen the wired error. Are you planning to release an official version with this change ?

jwaliszko commented 9 years ago

Hi,

The updated version (2.2.7) is already on NuGet.

pawelpabich commented 9 years ago

Awesome! I will update tomorrow.