TaskScheduler race condition in 3.70 (includes repro unit test)

Hello all, I looked through the source of the new 3.70 release (very excited about the new Azure scaling support!).  I believe there is a race condition in the implementation of the Task.cs Execute() method.  Specifically, there is no lock preventing the LeasedUntilUtc value from being modified in between the time it is read and written back to the redis cache.  Therefore, if multiple machines read an empty lease value, they will all simultaneously enter the lock and execute the scheduled task simultaneously.

Redis specifically has the "NX option" which sets a lock value ONLY if the value is presently empty to handle this situation (see http://redis.io/topics/distlock), so I believe that is the proper solution instead of a transaction lock.

I have attached a patch to the raw 3.70 source with a new WebFarmTaskRaceCondition unit test which will reproduce this behavior.  This test explicitly forces Tasks running on two different threads to enter the lock simultaneously.  You can see both of them run concurrently and print

Executing TaskRace on Thread 10....
Executing TaskRace on Thread 11....

in the output.

Patch at https://gist.github.com/anonymous/f783a4793f039f46ef3b/raw/489779eeb07d0d1efd88a57de80ed72f9af593e8/race_condition_repro.patch

To apply the patch

1. From the downloaded 3.70 source:
~/Downloads/nopcommerce370/nopCommerce_3.70_Source$ patch -p2 -l < race-condition-repro.patch

2. From git:
git checkout f6b7de7 # checkout the 3.70 release
git checkout -b race-condition-repro
git apply < race-condition-repro.patch

Thanks,
Matt

Hi Matt,

Great! Thanks a lot for this contribution and reporting.

Hi,

any idea when this issue will be fixed and has there already been created a work item for it?

Thanks!

Br,
Johann

Hi Johann,

Sure. Please find this work item here

Hi Andrei,

thanks a lot.

Br,
Johann

We have investigated this problem.
And now for the Redis we use one of the library that implemented distributed locks algorithm. You can see changes in this commit.
Thanks for the suggestion and contribution.

Thank you for investigating! I will try to review this ASAP.