Dataloaders broken since v0.17.50 and merge of #3203

[Metamogul]

What happened?

Merging #3203 introduced a new behavior where marshaling list types now does not run concurrently anymore. However this breaks the use of dataloaders (#3203 (comment)).

What did you expect?

I discovered this new behavior accidentally when one of our pipelines failed and I started to debug the failing test. The service in question heavily relies on dataloaders, loosing them or changing our entire schema don't seem to be adequate solutions. The need to add the new directive everywhere to preserve the established concurrent behavior also introduces a lot of potential for accidentally breaking new or existing loaders. Finally I think this affects a lot more devs and I'd not expect such breaking changes in a big project.

Minimal graphql.schema and models to reproduce

See linked comment above.

versions

>= v0.17.50

Possible resolution

I suggest reimplementing the change in such a way that the new behavior is opt-in, e.g. using a new @Sequential directive, instead of making the old behavior opt-in. This way the change would be non-breaking for current users of dataloaders.

[werjo-gridfuse]

I 100% agree with @Metamogul.

If you look at the GraphQL spec https://spec.graphql.org/October2021/#sec-Normal-and-Serial-Execution it tells that parallel execution is considered the "normal" behaviour" except for mutations. Mutations must be executed sequential, because there may be side effects.

I think graphqlgen should implement the spec by default and not putting default or "normal" behaviour behind some directive or configuration.

[krupyansky]

@Metamogul @werjo-gridfuse @StevenACoffman I made fix

#3286

Please give me review

[razorness]

Thank you for opening this Issue. And @krupyansky, thank you for your quick response.

There are only 2 aspects where concurrent execution comes into place:

• field resolver
• lists

I think this is a question of optimization. If you have a huge lists and no benefit of concurrent execution, you should have the opportunity to opt-in for sequential marshalling by a directive on the field which is holding the list. In this way, your schema is still readable.

I guess, the field resolver thing can be tuned by the number of configured field resolvers at object level. A single field resolver doesn't need its own go routine.
However this needs benchmarks to see if there is more than micro management with low benefit.

[StevenACoffman]

@Metamogul @werjo-gridfuse @krupyansky I reverted the #3203 and cut a new release v0.17.52

I'm sorry for the fact that I missed this disruption. Please let me know how I can improve the dataloader test harness so this sort of disruption does not happen in the future.

If someone would like to make a completely opt-in change (like adding a @sequential directive for new behavior) or prove the change could be fixed and provide a huge performance benefit, I would be willing to consider that.

I have had a hard time understanding you @krupyansky and that is my fault for not knowing more languages than just English. However, if you have access to someone who can help you translate it might make collaboration smoother between us.

[Metamogul]

Thanks you @StevenACoffman for the super quick fix! However I think nobody is to blame here, and where awesome features are built from people's free time mistakes can and will happen. I'm grateful to everybody contributing to this project.
Unfortunately I'm not familiar enough with your testing procedures and also lack the time to dive deeper into them to suggest a change that would have discovered this problem. On our side the failing loader was discovered because we had a second bug in the mix, leading to different results being returned depending on a loader being involved or not.

[StevenACoffman]

@Metamogul honestly, that is a great test that you could contribute. Have a simple dataloader supply different data than if it's not involved.