Race condition in WebApi ProductsController can lead to Duplicate key error in IX_UrlRecord_Slug

[Algorithman]

Describe the bug I have a multithreaded uploading tool which uploads multiple products at once. What's happening is that if you upload 2 (or more) products with the same name it can happen that the slug is created at the same time. In the ProductsController.cs UpdateSlug method you can have multiple threads validate the slug for the same result which then leads to a Duplicate entry '<slugname>' for key 'IX_UrlRecord_Slug'. In the end you are left with an inconsistent state of the database. The Product is shown in the frontend, but it's link is pointing only to the root folder since there is no slug for the product available.

Since there is no scope applied to the Post method and UpdateSlug is preceded by a await Db.SaveChangesAsync(); you get an error message returned, but the product itself is created anyway, just without a UrlRecord entry.

To Reproduce Steps to reproduce the behavior: Quite hard to reproduce, but quick succession of webapi calls from a multithreaded application with products which have the same name does the trick for me.

Expected behavior The UpdateSlug method should be thread safe, multiple users could upload products via webapi and just hit send at the same time.

I know it is a quite sporadic issue and I probably can work around it, but I wanted to let you know.

Desktop (please complete the following information):

• OS: all
• Browser all

[Algorithman]

~~A simple retry mechanism in UpdateSlug or locking with a static object (on controller level) could be enough to fix this.~~

~~The same problem exists in every Post method which calls SmartOdataController.PostAsync and has the UpdateSlug method in the add parameter like Category and Manufacturer. Also Put and Patch are also affected in these classes.~~

Scratch that, the condition also can appear if someone for example edits a product/manufacturer/category changing it's slug there and a webapi call tries to add another product/manufacturer/category with the same slug. Maybe a AutoResetEvent in the UrlService?.

[mgesing]

Can you please check my last commit to see if it works in your Web API scenario? I have only checked it with a hard-coded test version of UrlService.ValidateSlugAsync.

[Algorithman]

I'll try it on friday probably. Need to fix up some other stuff first. As for the code itself (after a quick look) it will work with 2 concurrent threads, but if you have more it still can break. Lets assume 2 threads conflict and the catch block is executed. Thread number one is irrelevant since it saved and caused the thread two to throw an exception. Now if a 3rd thread comes in and validates its slug just before thread 2 reaches line 367, then 2nd thread validates also with a valid result. 3rd thread saves and you're in the same position again.

I think the only way out of this dilemma is to make the UpdateSlugAsync into a method which can only be run by one thread at a time. That's why I thought of the AutoResetEvent.

Then we could do something like this (not testet yet):

        private static AutoResetEvent updateSlugAutoResetEvent = new AutoResetEvent(true):
        protected async Task UpdateSlugAsync<T>(T entity)
            where T : ISlugSupported
        {
            await updateSlugAutoResetEvent.WaitOneAsync();
            try {
                var urlService = HttpContext.RequestServices.GetService<IUrlService>();
                var slugResult = await urlService.ValidateSlugAsync(entity, string.Empty, entity.GetDisplayName(), true);
                var urlRecord = await urlService.ApplySlugAsync(slugResult);
           } catch (Exception whatever)
           { ... if we need exception handling ... }
           finally {
               updateSlugAutoResetEvent.Set();
           }
        }

The second point is this: This only covers the WebApi, but lets say someone changes the slug in the backend and a WebApi request also comes in at the same time. -> Same problem. So the fix should be in the UrlService, not only in the OData controllers. Everywhere I looked for ApplySlugAsync it is always in the same fashion (only exception is CommitAsync in UrlServiceBatchScope):

var validateSlugResult = await manufacturer.ValidateSlugAsync(localized.SeName, localized.Name, false, localized.LanguageId);
await _urlService.ApplySlugAsync(validateSlugResult);

The SeoExtensions method then also take it to the UrlService (just with an additional EngineContext.Current.Scope.Resolve<IUrlService>()).

Most times this extension call is unnecessary, since the classes already have the UrlService injected.

So in my opinion it would be best to make a combined ValidateAndApplySlugAsync method (I didn't see ApplySlugAsync ever alone) and limit this to one thread only. As for the Batch commit in UrlServiceBatchScope, it could also use the gating mechanism in UrlService if it is done via a static AutoResetEvent.

I'll think about it some more this evening :)

[mgesing]

Chasing and avoiding index uniqueness violations in a multithreaded application always and everywhere sounds like a battle against windmills to me. The option of intentionally running into these errors and later providing missing slugs via a task would be more likeable to me. But let's wait and see what our boss has to say.