Concurrency error when updating a settings part for content type

[giannik]

From the admin a try to update/save the settings for a custom part In my content type.
When hitting Save button in the admin I get the following error :

Seems that it has to do with the recent Distributed cache pr (#5249) that was merged

This is the code in my settingsdisplay driver :
The updateAsync calls some contentItemDisplayManager.UpdateEditorAsync()
which i beleive is the culprit.

If i comment out this call then the error disappears.

 public class ActionsPanelPartSettingsDisplayDriver : ContentTypePartDefinitionDisplayDriver
    {
        private readonly IContentManager _contentManager;
        private readonly IServiceProvider _serviceProvider;
  
        public override async Task<IDisplayResult> UpdateAsync(ContentTypePartDefinition contentTypePartDefinition, UpdateTypePartEditorContext context)
        {
            if (!String.Equals(nameof(ActionsPanelPart), contentTypePartDefinition.PartDefinition.Name, StringComparison.Ordinal))
            {
                return null;
            }

            var contentItemDisplayManager = _serviceProvider.GetRequiredService<IContentItemDisplayManager>();

             var model = new ActionsPanelPartSettingsViewModel();
             await context.Updater.TryUpdateModelAsync(model, Prefix );
 

             for (var i = 0; i < model.Prefixes.Length; i++)
             {
                 var contentItem = await _contentManager.NewAsync(model.ContentTypes[i]);

                 contentItem.Weld(new FlowMetadata());

                var widgetModel = await    contentItemDisplayManager.UpdateEditorAsync(contentItem, context.Updater, context.IsNew, htmlFieldPrefix: model.Prefixes[i]);

                 model.FlowPartDisplayMode.Widgets.Add(contentItem);
            }
         
            context.Builder.WithSettings(new ActionsPanelPartSettings
                { FlowPartDisplayMode = model.FlowPartDisplayMode
               });

            return Edit(contentTypePartDefinition, context.Updater);
        }
    }

Any ideas @jtkech ?

[jtkech]

Yes there is a new checkConcurrency option that we now use when doing a YesSQL session save and that add an optimistic concurrency checking when commiting the session, this by using a new Version column in a document table.

This may happen when we update the same document (same Version value) concurrently

Did you see the same when updating another type part settings?

[giannik]

No , other part settings do not cause the issue.
Only this one which has a call to :

await    contentItemDisplayManager.UpdateEditorAsync()

Is there any workaround?
Thanks.

[giannik]

But also noticed in latest dev when enabling /disabling features from the admin i get the following, might be related :

I am using sql server express as db

[jtkech]

Sorry i was not able to repro

Can you share a testing app so that i could repro

About the MARS issue most of the time it is due to a non awaited async call in a driver / handler / index provider

You are storing content type definitions in the database, not in the ContentDefinition.json file, right?

It is uncommon that a type part settings hold fake content items ;) Creating them use the content type definitions that you are updating in your type part definition driver, hmm, but it doesn't explain the issue.

[giannik]

Found out the culprit with the MARS issue. It was a non awaited call in one of my modules, as you said.
Closing the issue, eventually will figure out the type part settings and will report my findings.
Thank you @jtkech

[deanmarcussen]

@jtkech reopening this as am getting similar MARS related issues.

Here's a repro

• sql server (local)
• blog recipe
• export content definitions
• disable file content definition store
• import content definitions
• enable module with multiple migrations

MARS error.
When migration is tweaked a bit can turn into an open data reader exception (so it is like we had last time with missing awaits.

Here's a sample migration I can get to fail 3/4 times

        public int Create()
        {
            _contentDefinitionManager.AlterPartDefinition("SeoMetaPart", builder => builder
                .Attachable()
                .WithDescription("Provides a part that allows seo meta descriptions to be applied to a content item.")
                .WithField("DefaultSocialImage", field => field
                    .OfType("MediaField")
                    .WithDisplayName("Default social image")
                    .WithSettings(new MediaFieldSettings { Multiple = false }))
                .WithField("OpenGraphImage", field => field
                    .OfType("MediaField")
                    .WithDisplayName("Open graph image")
                    .WithSettings(new MediaFieldSettings { Multiple = false }))
                .WithField("TwitterImage", field => field
                    .OfType("MediaField")
                    .WithDisplayName("Twitter image")
                    .WithSettings(new MediaFieldSettings { Multiple = false }))
            );

            return 1;
        }

        public async Task<int> UpdateFrom1Async()
        {
            await _recipeMigrator.ExecuteAsync("socialmetasettings.recipe.json", this);
            return 2;
        }

socialmetasettings.recipe.json.zip

if it doesn't fail this helps make it fail

        public async Task<int> UpdateFrom1Async()
        {
            await Task.Delay(1000);
            await _recipeMigrator.ExecuteAsync("socialmetasettings.recipe.json", this);
            await Task.Delay(1000);
            return 2;
        }

Sometimes it seems like it needs to be delayed (I saw this first on a remote server connection to azure, which is always slower)

Adapting the migration to use
CreateAsync and the AlterPartDefinitionAsync methods was more likely to turn it into an open data reader exception.

Buried underneath the open data reader exception (was only visible in the logs), is the ConcurrencyException

Ping me if I can help with anymore details

P.S. doesn't happen with sqlite at all...

[jptissot]

I ran into this as well this morning.

Repro steps:

• Create a module with a migration that adds a role via a recipe migration and content types via the ContentDefinitionManager.
• If the recipe is executed before the ContentDefinitionManager, the exception is shown. If the recipe is executed after, no exception occur.

This also happens with feature dependencies.

• Feature 1 creates a migration that sets a role via the RoleManager<IRole> or a recipe step
• Feature 2 uses the ContentDefinitionManager

Exception shown:

ConcurrencyException: The document could not be updated as it has been changed by another process.
YesSql.Commands.UpdateDocumentCommand.ExecuteAsync(DbConnection connection, DbTransaction transaction, ISqlDialect dialect, ILogger logger)

DocumentStoreCommitException: The 'ContentDefinitionRecord' could not be persisted and cached as it has been changed by another process.

If this line is moved at the top of the method, enabling the module fails.

Enabling this feature directly fails as well because it is dependent on the hackathon feature.

Enabling them one at a time, in order works. (Hackathon, then Judging)

[jtkech]

Yes, maybe different issues here but not so easy to repro, may be good to have a testing app to be able to repro each individual issue, and / or don't hesitate to ping me on skipe.

But "Good" new, I just could reproduce the MultipleActiveResultSets issue by adding a role through a migration recipe, maybe because roles are managed differently because of the related aspnetcore services, so at some point before doing a mutation we still first use a GetOrCreateImmutableAsync() that does a session Detach() on the entity to get an isolated object, so maybe in that case the YesSql Version is double incremented

Note: ConcurrencyException may also be thrown if the YesSql cmd fails to update (updateCount is not equal to 1).

Update: So yes if i remove the following

        //if (document != null)
        //{
        //    _session.Detach(document);
        //    return (true, document);
        //}

I don't get the MultipleActiveResultSets issue, but i now get the following (with database content definition)

The 'ContentDefinitionRecord' could not be persisted and cached as it has been changed by another process.

So okay, i'm now able to repro both issues, i'm working on ;)

For now, about the ConcurrencyException there is a workaround, the CheckConcurrency option is true by default but esay to set it to false, idem for the CheckConsistency option that redo a GetOrCreateImmutableAsync() at the end of an update and that may explain a different issue (but maybe related to the same unique issue). This can be changed in DocumentOptionsSetup.cs, note: can be also changed only for a given type through the appsettings with a section named with the document type fullName.

Can be also overriden by code easily per document type

Update: the 2 exceptions maybe interrelated, but setting only CheckConsistency to false (not CheckConcurrency) for the 2 following documents fixes the issues, at least in my local tests, so try this workaround

         // In the roles module startup
        services.Configure<DocumentOptions>(typeof(RolesDocument).FullName, options =>
        {
            options.CheckConsistency = false;
        });

         // In the content types module startup
        services.Configure<DocumentOptions>(typeof(ContentDefinitionRecord).FullName, options =>
        {
            options.CheckConsistency = false;
        });

But would be better to keep this option to true for these 2 documents, so i need to find exactly what happens.

[jtkech]

@jptissot @deanmarcussen

Just did a PR #7516, not sure at 100% so if you can give it a try to see if it fixes your issues

[giannik]

Tried the pr with my case , using in part type settings editor :

await    contentItemDisplayManager.UpdateEditorAsync()

but did not resolve issue.

[jtkech]

@giannik

Yes, i think it is another use case as NewAsync() is getting definitions that you are updating, so let's @jptissot and @deanmarcussen try their use cases first, then tomorrow i will try to understand what happens in your case.

[deanmarcussen]

@jtkech tried with my scenario.

Without your branch fails.
With your branch works :)

[jtkech]

@deanmarcussen good to know!

@jptissot can you try it to confirm when you will have time

@giannik i tried the same kind of loop using UpdateEditorAsync() in our FlowPartSettingsDisplayDriver, with sql server and type definitions in the database, i can't repro, it works even without my change in the related PR

So it seems that something else happens, maybe in a custom handler, I could help if you have a testing app to repro, in the meantime you could try this workaround

    services.Configure<DocumentOptions>(typeof(ContentDefinitionRecord).FullName, options =>
    {
        options.CheckConcurrency = false;
    });

[giannik]

thanks @jtkech Ill try to make a testing app in the weekend and send it here.
Oh, and i tried the workaround but same result.

[jtkech]

@giannik cool, thanks

@jptissot can you try #7516 if you have time, it works for @deanmarcussen, would be good to confirm it also fixes your issue ;)

[giannik]

@jtkech The error occurs from the workflows module here :

namespace OrchardCore.Contents.Workflows.Handlers
{
 public class ContentsHandler : ContentHandlerBase
    {
            public override Task UpdatedAsync(UpdateContentContext context)
         {
             return TriggerWorkflowEventAsync(nameof(ContentUpdatedEvent), context.ContentItem);
          }

    }
}

and specifically from this line when return is called :

OrchardCore/src/OrchardCore.Modules/OrchardCore.Workflows/Services/WorkflowManager.cs

Line 124 in 7a2e676

return;

[jtkech]

@jptissot can you try #7516 if you have time, it works for @deanmarcussen, would be good to confirm it also fixes your issue ;)

@giannik yes the NewAsync() and UpdateEditorAsync() in your loop in your ContentTypePartDefinitionDisplayDriver will trigger workflow events, i will think about this use case this night.

Do you have any workflow using a ContentUpdatedEvent?

[giannik]

No i dont have workflow defintion using *ContentUpdatedEvent. * As seen in the return call which gets called when no workflow matches the event.

…

On Fri, Nov 6, 2020 at 8:47 PM Jean-Thierry Kéchichian < ***@***.***> wrote: @jptissot <https://github.com/jptissot> can you try #7516 <#7516> if you have time, it works for @deanmarcussen <https://github.com/deanmarcussen>, would be good to confirm it also fixes your issue ;) @giannik <https://github.com/giannik> yes the NewAsync() and UpdateEditorAsync() in your loop in your ContentTypePartDefinitionDisplayDriver will trigger workflow events, i will think about this use case this night. *Do you have any workflow using a ContentUpdatedEvent*? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7449 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALSLNK4JA5YM26BF32WXLTSORAD3ANCNFSM4TFGMREQ> .

[jtkech]

@giannik okay thanks, i assume this is why i was not able to repro as i didn't enable the WorkFlows feature, hmm maybe i've some ideas, i will re-try it this night.

[jtkech]

@giannik "Good" new, just tried, now i can repro ;)

What i can see right now, is that on each loop TriggerWorkflowEventAsync() really does 2 database queries, this is not the case when we get multiple times a content item or a document as type definitions where we return an already loaded object.

[jtkech]

@giannik okay i think i found a little issue on YesSql side

There is a case where the document version is incremented but the related UpdateDocumentCommand is not enlisted, i will open an issue and maybe a PR on Yesql.

Meanwhile, for your use case you can use the following workaround in the OC.ContentTypes module startup

services.Configure<DocumentOptions>(typeof(ContentDefinitionRecord).FullName, options =>
{
    options.CheckConcurrency = false;
});

If you don't use the full source code, i can show you how to do it through the appsettings.json, let me know

Update: sebastienros/yessql#287

Update:

I built YesSql locally and made packages in a local feed, so i could try my YesSql updates, I can confirm that it fixes your issue but you have to wait that it get merged on YesSql and then the new version referenced by OC. Note: For infos, to be able to build OC in this context i needed to do some adaptations as currently we don't reference the last dev of YesSql.

[giannik]

@jtkech thank you very very much.
I dont mind waiting for the yessql update.
Must buy you a beer.

[jtkech]

@giannik

It doesn't find tbl_ContentItemIndex, not related to a single shared Document

So better to open another issue.

Sorry I can't repro. Which setup recipe? Does it happen every time you create a tenant or only once?

---

So, maybe i will merge #7516 as it fixes the issue of @deanmarcussen

For your original issue it will be fixed through sebastienros/yessql#287

Merging #7516 will close this issue but feel free to open any new issue e.g. related to your last problem

[giannik]

sure , will open it as a new issue.

[jtkech]

@giannik

Okay i found a temporary fix for your original issue (not the last different one), i integrated it in #7516 so that we don't have to wait for an update of YsSql, see e7a1bc5

[jptissot]

@jtkech I just tested this today and it fixes the issue I was having.

[jtkech]

@jptissot cool, thanks for testing ;) it was important to confirm it, thanks again