Lazy GraphTypeInfo.TypeParameter errors

[K-Pavlov]

Upgrading to 2.4.0 is causing a lot unexpected erroneous behavior from the lazy GraphTypeInfo.TypeParameter From GraphTypeAdapter.ConstructInterfaceType I am getting

Exception message: Collection was modified; enumeration operation may not execute.

It seems to me that things there are happening in a strange order (almost like parallel execution, from the lazy init); I haven't created a repro case for this as it would be complex (I could do it); Is there such a big gain from the Lazy or could it just be reverted; If there is, a lot of test cases need to be written to verify that things work;

[K-Pavlov]

Commit introducing this https://github.com/graphql-dotnet/conventions/commit/9981659818c951e08b9d7b46f473e80e7c8b4e05; @tlil @BilyachenkoOY

[tlil]

Ah, sorry to hear that. @BilyachenkoOY, any chance you could put together a fix?

[BilyachenkoOY]

Looks strange, as Lazy constructed with LazyThreadSafetyMode.ExecutionAndPublication. I'll try to reproduce it ASAP, but having some steps/insights would be great.

[K-Pavlov]

@BilyachenkoOY We have a pretty big schema, with a lot of types, it's hard to pin-point the exact issue because it is similar to a concurrency issue; From the looks of it the possible types collection is modified while being iterated:

typeInfo
                .PossibleTypes
                .Select(t => DeriveType(t) as IObjectGraphType)

This is happening in ConstructInterfaceType; Debugging our schema and schema construction didn't give me much information, sorry about that. I have a stack trace:

ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
    Enumerator.MoveNextRare()
    Enumerator.MoveNext()
    WhereSelectListIterator`2.MoveNext()
    GraphTypeAdapter.ConstructInterfaceType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.GetComplexType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.DeriveType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.GetType(GraphTypeInfo typeInfo)
    WhereSelectListIterator`2.MoveNext()
    GraphTypeAdapter.ConstructOutputType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.GetComplexType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.DeriveType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.DeriveField(GraphFieldInfo fieldInfo)
    WhereSelectListIterator`2.MoveNext()
    GraphTypeAdapter.DeriveFields(GraphTypeInfo typeInfo, IComplexGraphType graphType)
    GraphTypeAdapter.ConstructOutputType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.GetComplexType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.DeriveType(GraphTypeInfo typeInfo)
    GraphTypeAdapter.DeriveSchema(GraphSchemaInfo schemaInfo)
    SchemaConstructor`2.Build(TypeInfo[] schemaTypes)
    SchemaConstructor`2.Build(Type[] schemaTypes)
    GraphQLEngine.BuildSchema(Type[] types)

[K-Pavlov]

I can also try to reproduce it, but I am not really sure when I will have the time for that

[tlil]

@BilyachenkoOY did you get anywhere with this?

[BilyachenkoOY]

@tlil unfortunately - not :( I was not able to reproduce it or find right configuration for the case. I've also updated the lib in my project which contains hundreds of types, interfaces and some manually constructed GraphTypeInfo's and no errors occurred too.

[tlil]

I can also try to reproduce it, but I am not really sure when I will have the time for that

@K-Pavlov, can you hit us up when you get time to look at it / reproduce it? Would be good to get this one resolved. :)

[K-Pavlov]

@tlil @BilyachenkoOY Alright, I will be starting work on this soon

[K-Pavlov]

@tlil @BilyachenkoOY Managed to make a small repro case:

    internal class Program
    {
        private static async Task Main(string[] args)
        {
           await GraphQLEngine.New<BugReproQuery>()
                .NewExecutor()
                .Execute(); 
        }
    }

    public class BugReproSchema
    {
        public BugReproQuery Query { get; }
    }

    public class BugReproQuery
    {
        public Task<NonNull<Connection<NonNull<Test1>>>> Test()
        {
            return Task.FromResult(new NonNull<Connection<NonNull<Test1>>>());
        }

        public class Test1 : INode
        {
            public Id Id => throw new NotImplementedException();

            public Task<NonNull<IEnumerable<NonNull<Test2>>>> Test2() =>
                Task.FromResult(new NonNull<IEnumerable<NonNull<Test2>>>());
        }

        public class Test2 : INode
        {
            public Id Id => throw new NotImplementedException();
        }
    }

This throws:

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.

[BilyachenkoOY]

@K-Pavlov OK, it fails because I missed that lists of structs (Cursor in this case) are considered as primitives too. Moving EnsureTypeParameterInitialized from ObjectReflector.cs#L112 right after type = _typeCache.AddEntity(typeInfo, new GraphTypeInfo(_typeResolver, typeInfo)) should solve the situation.

I can send a PR with fix but I'll need some free time to add some unit tests.

[BilyachenkoOY]

@tlil I've simplified test-case a little bit (upd also there was a bug with implementations discovering) and created a PR. @K-Pavlov It would be very great if you can approve that fix works on your project, so that we can be sure it is still the same case :)

[tlil]

Merged

[tlil]

Thanks for the fix!