GraphQL session MARS/DataReader is open problem

[tropcicstefan]

Describe the bug

Resolving graphQLFields of content part through DataLoader causes MARS or Datareader is open exception.

To Reproduce

Latest dev branch
Make content Part with two taxonomy fields(JournalistCategory, ArticleCategory). Make custom GraphQLObjectType that exposes those data as two Field<ListGraphType<ContentItemInterface>, ContentItem[]> and resolve those fields querying DB for linked taxonomy and parsing it to get that specific ContentItem. It appears as having two or more graphQLFields when resolving through ISession is too much

`

public class CustomCategoryObjectType : ObjectGraphType<CustomCategoryPart>
{
    public CustomCategoryObjectType(IStringLocalizer<CustomCategoryObjectType> S)
    {
        Name = "CustomCategory";
        Description = S["Alternative part definition"];
        Field<ListGraphType<ContentItemInterface>, ContentItem[]>()
            .Name("journalistMetadata")
            .Description("Exposed journalist taxonomy metadata")
            .PagingArguments()
            .ResolveAsync(context =>
            {
                var serviceProvider = context.ResolveServiceProvider();
                var accessor = serviceProvider.GetRequiredService<IDataLoaderContextAccessor>();
                var session = serviceProvider.GetService<ISession>();

                var contentItemLoader = accessor.Context.GetOrAddBatchLoader<string, ContentItem>("GetJournalistsById",
                    ci => LoadTaxContentItems(ci, session, Const.Taxonomy.Journalist));
                return contentItemLoader.LoadAsync(context.Page(context.Source.JournalistCategory.TermContentItemIds));
            });
        Field<ListGraphType<ContentItemInterface>, ContentItem[]>()
            .Name("categoryMetadata")
            .Description("Exposed category taxonomy metadata")
            .PagingArguments()
            .ResolveAsync(context =>
            {
                var serviceProvider = context.ResolveServiceProvider();

                var accessor = serviceProvider.GetRequiredService<IDataLoaderContextAccessor>();
                var session = serviceProvider.GetService<ISession>();

                var contentItemLoader = accessor.Context.GetOrAddBatchLoader<string, ContentItem>("GetCategoryById",
                    ci => LoadTaxContentItems(ci, session, Const.Taxonomy.Category));
                return contentItemLoader.LoadAsync(context.Page(context.Source.ArticleCategory.TermContentItemIds));
            });
    }

    private async Task<IDictionary<string, ContentItem>> LoadTaxContentItems(
        IEnumerable<string> contentItemIds,
        ISession session,
        string taxItemId)
    {
        if (contentItemIds is null || !contentItemIds.Any())
        {
            return new Dictionary<string, ContentItem>();
        }

        var tax = await session
            .Query<ContentItem, ContentItemIndex>(y =>
                y.ContentItemId == taxItemId
                && y.Published)
            .FirstOrDefaultAsync();

        if (tax is null)
        {
            return new Dictionary<string, ContentItem>();
        }

        var contentItemsLoaded = tax.As<TaxonomyPart>().Terms.Where(x => contentItemIds.Contains(x.ContentItemId)).ToList();

        return contentItemsLoaded.ToDictionary(k => k.ContentItemId, v => v);
    }
}

`
...call GraphQL with both fields in query

query MyQuery { article(first: 10) { customCategory { categoryMetadata { displayText } journalistMetadata { displayText } } } }

Expected behavior

#4844 if we should use locking resolver why aren't content picker field and list part using it. probably type with list part and cpf can cause the same exception

Splitting fields in separate parts makes this issue occur less frequently

[carlwoodhouse]

Hi you need to use ‘ResolveLockedAsync’ instead of ResolveAsync currently when making db calls in resolvers. We’re hoping to change this behaviour with the graphqldotnet 4 upgrade we’re working on currently.

…

On Fri, 30 Apr 2021 at 18:05, tropcicstefan ***@***.***> wrote: Describe the bug Resolving graphQLFields of content part through DataLoader causes MARS or Datareader is open exception. To Reproduce Latest dev branch Make content Part with two taxonomy fields(JournalistCategory, ArticleCategory). Make custom GraphQLObjectType that exposes those data as two Field<ListGraphType<ContentItemInterface>, ContentItem[]> and resolve those fields querying DB for linked taxonomy and parsing it to get that specific ContentItem. It appears as having two or more graphQLFields when resolving through ISession is too much ` public class CustomCategoryObjectType : ObjectGraphType<CustomCategoryPart> { public CustomCategoryObjectType(IStringLocalizer<CustomCategoryObjectType> S) { Name = "CustomCategory"; Description = S["Alternative part definition"]; Field<ListGraphType<ContentItemInterface>, ContentItem[]>() .Name("journalistMetadata") .Description("Exposed journalist taxonomy metadata") .PagingArguments() .ResolveAsync(context => { var journalist = context.Source.JournalistCategory.TermContentItemIds.ElementAtOrDefault(0); if (journalist == null) { return null; } var serviceProvider = context.ResolveServiceProvider(); var accessor = serviceProvider.GetRequiredService<IDataLoaderContextAccessor>(); var session = serviceProvider.GetService<ISession>(); var contentItemLoader = accessor.Context.GetOrAddBatchLoader<string, ContentItem>("GetJournalistsById", ci => LoadTaxContentItems(ci, session, Const.Taxonomy.Journalist)); return contentItemLoader.LoadAsync(context.Page(context.Source.JournalistCategory.TermContentItemIds)); }); Field<ListGraphType<ContentItemInterface>, ContentItem[]>() .Name("categoryMetadata") .Description("Exposed category taxonomy metadata") .PagingArguments() .ResolveAsync(context => { var category = context.Source.ArticleCategory.TermContentItemIds.ElementAtOrDefault(0); if (category == null) { return null; } var serviceProvider = context.ResolveServiceProvider(); var accessor = serviceProvider.GetRequiredService<IDataLoaderContextAccessor>(); var session = serviceProvider.GetService<ISession>(); var contentItemLoader = accessor.Context.GetOrAddBatchLoader<string, ContentItem>("GetCategoryById", ci => LoadTaxContentItems(ci, session, Const.Taxonomy.Category)); return contentItemLoader.LoadAsync(context.Page(context.Source.ArticleCategory.TermContentItemIds)); }); } private async Task<IDictionary<string, ContentItem>> LoadTaxContentItems( IEnumerable<string> contentItemIds, ISession session, string taxItemId) { if (contentItemIds is null || !contentItemIds.Any()) { return new Dictionary<string, ContentItem>(); } var tax = await session .Query<ContentItem, ContentItemIndex>(y => y.ContentItemId == taxItemId && y.Published) .FirstOrDefaultAsync(); if (tax is null) { return new Dictionary<string, ContentItem>(); } var contentItemsLoaded = tax.As<TaxonomyPart>().Terms.Where(x => contentItemIds.Contains(x.ContentItemId)).ToList(); return contentItemsLoaded.ToDictionary(k => k.ContentItemId, v => v); } } ` Expected behavior @carlwoodhouse <https://github.com/carlwoodhouse> here #4844 <#4844> says that Dataloaders should be enough to not cause this issue — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9330>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEEUCLI2Z3EYU34EJK3TLLPOBANCNFSM434ZEVRQ> .

[tropcicstefan]

But LockedAsyncFieldResolver cannot be used in combination with DataLoader, right?

[carlwoodhouse]

it should be able too, all it does is force a lock to stop parallel db calls (so eliminating the mars issue) - however looking through the code as you say the list parts should use it.

Honestly its a long time since i looked at it so memory is a bit hazy .... and im away atm so cant look properly on phone. but can investigate when im back.

(again it will be fixed anyway by the graphqldotnet uplift we're working on which has much better first class di support)

[tropcicstefan]

it should be able too, all it does is force a lock to stop parallel db calls (so eliminating the mars issue)

Changing ResolveAsync to ResolveLockedAsync with DataLoaders appears to cause a deadlock because request just "hangs".

...looking forward for upgrade

[carlwoodhouse]

Ah ok that actually makes sense .. In the mean time You could try resolving the session in the method your passing to the data loader instead of passing the session (reverting to loadasync); not sure it will work but those you run sync after all the fields have resolved in parallel which is what’s causing the issue

…

On Fri, 30 Apr 2021 at 22:01, tropcicstefan ***@***.***> wrote: it should be able too, all it does is force a lock to stop parallel db calls (so eliminating the mars issue) Changing ResolveAsync to ResolveLockedAsync with DataLoaders appears to cause a deadlock because request just "hangs". ...looking forward for upgrade — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEEUAAALVR3ISJI6XTLTLMLBZANCNFSM434ZEVRQ> .

[tropcicstefan]

I tried passing context or service provider and in both cases error is still occurring.
I'll switch back to LockedAsyncFieldResolver but I had similar problem with it back in october with this piece of code, when querying content type that has two resolvers like below (with different query). Do you see any problem here?
It occurred only in production in busy period of they day, for news portal.

.ResolveLockedAsync(async g =>
      {
          var serviceProvider = g.ResolveServiceProvider();
          var queryService = serviceProvider.GetRequiredService<IQueryService>();
      
          var queryParametersDict = new Dictionary<string, object>() {
              { "articleAlias", g.GetArgument<string>("articleAlias") }
          };
          var queryParam = JsonConvert.SerializeObject(queryParametersDict);
      
          return await queryService.GetItemsQueryAsync(Query.SummarySections, queryParam);
      })

public class QueryService : IQueryService
    {
        private readonly IQueryManager _queryManager;

        public QueryService(IQueryManager queryManager)
        {
            _queryManager = queryManager;
        }

        public async Task<IEnumerable<ContentItem>> GetItemsQueryAsync(string queryName, string queryParam)
        {
            var query = await _queryManager.GetQueryAsync(queryName);

            if (query == null)
            {
                throw new ArgumentException("There is no query with that name");
            }

            var queryParametersDict = queryParam != null ?
                JsonConvert.DeserializeObject<Dictionary<string, object>>(queryParam)
                : new Dictionary<string, object>();

            var result = await _queryManager.ExecuteQueryAsync(query, queryParametersDict);
            var items = result.Items
                .Cast<ContentItem>();

            return items;
        }
    }

[sebastienros]

Try to lock on the session object by yourself, and also use GetAwaiter().GetResult() in the LoadTaxContentItems method though this might cause a thread pool exhaustion on high loads. This looks like this method is not awaited by GraphQL.NEt which is something we had to deal with by the past, so hopefully the new version fixes it.

[Skrypt]

@tropcicstefan Is this issue still relevant?

[MikeKry]

So if this issue is still active and cant be fixed by adding LockedAsyncFieldResolver, won't we finish the PR for GQL4 #9087 which should address this?

Big problem is, that this issue can occur unexpectedly - it happens only in combination with some field which is using dataloader (e.g. TaxonomyField), so it may not be found in development of custom field and then it is complicated to find root cause of error, because error usually does not happen in your custom field, but points to TaxonomyField.. how should you find it when you have several custom fields..

I would love to help finish that, but I lack enough information about dataloaders etc. My primary focus is to use OC as headless and I feel that this should be resolved, not sure if most people use OC as monolith instead of headless?

[Skrypt]

Feel free to open a Pull Request that targets ag/graphql4 branch.

[MikeKry]

@Skrypt

I did some work on that PR too, but I am stuck, because there are some problems with dataloaders (n+1 issue, this problem..) that I do not have information about (how to test them e.g.) and seems like nobody will provide info - I do not know how to fix something I have no info about. Is there a way to contact someone who knows more about it directly, so I could potentially fix it?

[tropcicstefan]

@MikeKry
If you are referencing @carlwoodhouse about n+1 problem in your pr, I think he just said that dataloaders should be tested. You can test dataloaders with content item(articles) with content picker field(linked articles) and startup SQL Server profiler. In there you should see only one query regarding content picker field. So if you query for 10 articles there should be 1 query for linked articles with long where statement, and not 10 queries with short where :D

Regarding this issue:
My assumption was I need ResolveLockedAsync when using ISession. Truth is it depends, when you want to resolve field by field(introducing n+1), without dataloader, you should use ResolveLockedAsync, but with dataloaders you should use ResolveAsync and lock inside dataloader, in delegate when you use ISession.

So way to resolve this issue is either by:
a) updating docs and saying hey when writing custom graphql fields with dataloader lock on ISession, update current uses of dataloader i.e. ContentPickerField with locking.
b) write framework implementation of IDataLoader binded to YesSQL with lock on ISession

...would like to have another pair of eyes on my take

[carlwoodhouse]

I actually think with the changes in gql dotnet we can remove the locks .. and use the serial data execution strat we used too but removed because it didn’t support dataloaders .. which would fix these issues .. supposedly the dataloaders now supports it. Unfortunately I don’t really have the time to look into it myself at the moment ..

…

On Mon, 21 Mar 2022 at 15:40, tropcicstefan ***@***.***> wrote: @MikeKry <https://github.com/MikeKry> If you are referencing @carlwoodhouse <https://github.com/carlwoodhouse> about n+1 problem in your pr <#9087>, I think he just said that dataloaders should be tested. You can test dataloaders with content item(articles) with content picker field(linked articles) and startup SQL Server profiler. In there you should see only one query regarding content picker field. So if you query for 10 articles there should be 1 query for linked articles with long where statement, and not 10 queries with short where :D Regarding this issue: My assumption was I need ResolveLockedAsync when using ISession. Truth is it depends, when you want to resolve field by field(introducing n+1), without dataloader, you should use AsyncResolver, but with dataloaders you should use ResolveAsync and lock inside dataloader, in delegate when you use ISession. So way to resolve this issue is either by: a) updating docs and saying hey when writing custom graphql fields with dataloader lock on ISession, update current uses of dataloader i.e. ContentPickerField with locking. b) write framework implementation of IDataLoader binded to YesSQL with lock on ISession — Reply to this email directly, view it on GitHub <#9330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEHOXT27XNLX3CA2KVTVBCKADANCNFSM434ZEVRQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[carlwoodhouse]

^^ the patch notes of the new version suggest so anyway On Mon, 21 Mar 2022 at 17:14, Carl Woodhouse ***@***.***> wrote:

…

I actually think with the changes in gql dotnet we can remove the locks .. and use the serial data execution strat we used too but removed because it didn’t support dataloaders .. which would fix these issues .. supposedly the dataloaders now supports it. Unfortunately I don’t really have the time to look into it myself at the moment .. On Mon, 21 Mar 2022 at 15:40, tropcicstefan ***@***.***> wrote: > @MikeKry <https://github.com/MikeKry> > If you are referencing @carlwoodhouse <https://github.com/carlwoodhouse> > about n+1 problem in your pr > <#9087>, I think he just > said that dataloaders should be tested. You can test dataloaders with > content item(articles) with content picker field(linked articles) and > startup SQL Server profiler. In there you should see only one query > regarding content picker field. So if you query for 10 articles there > should be 1 query for linked articles with long where statement, and not 10 > queries with short where :D > > Regarding this issue: > My assumption was I need ResolveLockedAsync when using ISession. Truth is > it depends, when you want to resolve field by field(introducing n+1), > without dataloader, you should use AsyncResolver, but with dataloaders you > should use ResolveAsync and lock inside dataloader, in delegate when you > use ISession. > > So way to resolve this issue is either by: > a) updating docs and saying hey when writing custom graphql fields with > dataloader lock on ISession, update current uses of dataloader i.e. > ContentPickerField with locking. > b) write framework implementation of IDataLoader binded to YesSQL with > lock on ISession > > — > Reply to this email directly, view it on GitHub > <#9330 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AADRUEHOXT27XNLX3CA2KVTVBCKADANCNFSM434ZEVRQ> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[MikeKry]

I'll try to look at it, in meantime I found another problem which should be looked upon while resolving this issue (but maybe I just need some lock here? trying to solve for now by using concurrent collections):

When calling ResolveAsync and using dataloder with casting to part:

I get concurrent exception

System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. 
   A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
    at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
         at System.Collections.Generic.Dictionary`2.set_Item(TKey key, TValue value)
            at OrchardCore.ContentManagement.ContentExtensions.Get(ContentElement contentElement, Type contentElementType, String name)
               at OrchardCore.ContentManagement.ContentExtensions.Get[TElement](ContentElement contentElement, String name)
                 at OrchardCore.ContentManagement.ContentItemExtensions.As[TPart](ContentItem contentItem)

It happens inside .As<> extension method,,

Edit:
This will be resolved by removing dataloaders

[sebastienros]

Are data loaders invoked in parallel threads? That would explain the issue. If it's configurable it should be "serial". This is a common issue we had years ago with the defaults in GrapQL.NET and we explicitly use serial mode for some settings since then.

[sebastienros]

Moving to backlog since there is some work going on with upgrading the library.

[MikeKry]

@sebastienros

I looked into this during analysis while doing changes in GQL4 update

I think it might be also caused by cache - dataloaders by default return cached result for same key, so it is possible that it is being used by multiple functions.

There could be two solutions
a/ create full dataloader for GetOrAddPublishedContentItemByIdDataLoader and disable cache
b/ use some concurrent collection instead of loopuk in DataLoaderExtensions.LoadPublishedContentItemsAsync

If you agree, I can try one (or both) of them in #11499

[MikeKry]

I ran into another issue, which seems that it might not be related directly to dataloaders.. Happens only sometimes on production env. maybe when there are multiple requests at same time.

It is again related to .As<> (could there be some bug? I am running on OC 1.3), but no dataloaders involved inside LayerQueryObjectType. There are content pickers inside widgets, so it may be same issue, just hidden under this error message.

  "errors": [
    {
      "message": "GraphQL.ExecutionError: Error trying to resolve widgets.\r\n ---> System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.\r\n   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)\r\n   at System.Collections.Generic.Dictionary`2.set_Item(TKey key, TValue value)\r\n   at OrchardCore.ContentManagement.ContentExtensions.Get(ContentElement contentElement, Type contentElementType, String name)\r\n   at OrchardCore.ContentManagement.ContentExtensions.Get[TElement](ContentElement contentElement, String name)\r\n   at OrchardCore.ContentManagement.ContentItemExtensions.As[TPart](ContentItem contentItem)\r\n   at OrchardCore.Layers.GraphQL.LayerQueryObjectType.<>c__DisplayClass0_0.<.ctor>b__5(ContentItem item)\r\n   at System.Linq.Enumerable.WhereListIterator`1.MoveNext()\r\n   at GraphQL.Execution.ExecutionStrategy.SetArrayItemNodes(ExecutionContext context, ArrayExecutionNode parent)\r\n   at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node)\r\n   --- End of inner exception stack trace ---",
      "locations": [
        {
          "line": 61,
          "column": 5
        }
      ],
      "path": [
        "siteLayers",
        "0",
        "widgets"
      ],
      "extensions": {
        "code": "INVALID_OPERATION"
      }
    }
  ]

[jtkech]

Yes if shared objects are mutated in concurrent requests they would need to be thread safe or would need e.g. to retrieve clones from the cache, but still possible that it happens in the same request scope if there are non awaited tasks, in that case scope objects can be still concurrently mutated.

As mentioned in other issues, most of the time we have MARS issues it is due to non awaited tasks. Related to GraphQl just saw the following using a Task.WhenAll(tasks), a WhenAll() waits that all tasks complete but tasks are not waiting for each others. So if related, would need to use e.g. a simple foreach loop.

OrchardCore/src/OrchardCore/OrchardCore.Apis.GraphQL.Abstractions/Extensions/DataLoaderExtensions.cs

Lines 10 to 20 in 2c2696d

	public static Task<T[]> LoadAsync<TKey, T>(this IDataLoader<TKey, T> dataLoader, IEnumerable<TKey> keys)
	{
	var tasks = new List<Task<T>>();
	foreach (var key in keys)
	{
	tasks.Add(dataLoader.LoadAsync(key));
	}
	return Task.WhenAll(tasks);
	}

The above being used by ContentPickerFieldQueryObjectType, do you have ContentPickerFields?

Maybe related

[jtkech]

Hmm, maybe a good chance that the usage of Task.WhenAll(tasks) in ContentPickerFieldQueryObjectType explains the MARS and collection accesses issues, this is the only place where I found a remaining Task.WhenAll(tasks).

I think it is worth to try, I may suggest a PR when I will have time

[MikeKry]

@jtkech

yes, for now I have seen it only in queries that contain (more than one) ContentPickerFields (or custom fields with same dataloader), so it might be this.

[tropcicstefan]

That is not a solution because how dataloaders work and that method isn't called anywhere.
Look at #9769

The solution for this issue can be resolved in graphql version update branch with SerialDocumentExecuter like this

public class SerialDocumentExecuter : DocumentExecuter
{
    private static IExecutionStrategy ParallelExecutionStrategy = new ParallelExecutionStrategy(); 
    private static IExecutionStrategy SerialExecutionStrategy = new SerialExecutionStrategy();
    private static IExecutionStrategy SubscriptionExecutionStrategy = new SubscriptionExecutionStrategy();

    protected override IExecutionStrategy SelectExecutionStrategy(ExecutionContext context)
    {
        return context.Operation.OperationType switch
        {
            OperationType.Query => SerialExecutionStrategy,
            OperationType.Mutation => SerialExecutionStrategy,
            OperationType.Subscription => SubscriptionExecutionStrategy,
            _ => throw new InvalidOperationException($"Unexpected OperationType {context.Operation.OperationType}"),
        };
    }
}

[jtkech]

@tropcicstefan

Yes the method I changed isn't called anywhere, this is the one defined just before that uses a Task.WhenAll() and that is called in ContentPickerFieldQueryObjectType, was too tired ;)

In fact I updated the wrong method, will update it tomorrow.

[jtkech]

@tropcicstefan Okay I better understand now, thanks.

@tropcicstefan @MikeKry So I will revert #11536, see my comment #11548 (comment), also about a possible solution.

[MikeKry]

I may try serialexecution as @tropicstefan suggested in update PR I created few days ago #11499, if nobody is already working on that. But I wont get to that until next friday.

…

16. 4. 2022 v 5:07, Jean-Thierry Kéchichian ***@***.***>:  @tropcicstefan Okay I better understand now, thanks. @tropcicstefan @MikeKry So I will revert #11536, see my comment #11548 (comment), also about a possible solution. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.