Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Non awaited task in GraphQL DataLoaderExtensions (#11536)" #11548

Merged
merged 1 commit into from
Apr 16, 2022

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Apr 16, 2022

This reverts commit 5ba889e.

@sebastienros I reverted #11536 because first I didn't updated the right method, the one that uses a Task.WhenAll(), anyway If I remove the Task.WhenAll in the right extension method it works for a CPField with only one item (only CPField uses this method) otherwise it was blocking, you already tried to do exactly the same in #9769.

I noticed that the Task.WhenAll is not used to do concurrent queries but to grab all needed Ids, maybe this is here that concurrent accesses are done on our ContentItem Elements, notice that I was not able to repro #9330. If I remove the Task.WhenAll() and add some await, it tries to do a query for each Id and it is blocking on the 2nd one.

As I read GraphQL is more intended to provide a single item or a collection but both by an unique key, to support a multiple keys input they introduced an extension that exactly does what we do, i.e. they also use a Task.WhenAll().

For ListPart we use a CollectionBatchLoader that returns a collection but each group set being related to an unique key, the ContainedPart.ListContentItemId. For a CPField I could mimic the behavior of a ListPart by scope caching a relation between a guid and the item Ids of a given CPField, then I used this uid to retrieve the items. It works but for now if 2 CPFields reference the same item, this item is rendered once.

Let me know if it is worth to work on it, maybe not as it seems that there is an incoming version upgrade of GraphQL.

@jtkech jtkech merged commit 126424f into main Apr 16, 2022
@jtkech jtkech deleted the jtkech/data-loader branch April 16, 2022 03:07
@tropcicstefan
Copy link
Contributor

Don't think that is a solution either. Because the essence of a problem is in parallel resolving of graphql field. So if a graphql query contains listpart and cpf on same level they will execute in parallel and cause MARS problem.

@jtkech
Copy link
Member Author

jtkech commented Apr 21, 2022

@tropcicstefan

Ah okay, do you mean that we have the same issue with ListPart? Hmm, when I will have time I'll try to see more precisely what we are doing and also what GraphQL is doing on its side, interesting ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants