Including collection navigation after optional navigation throws NRE with async in 2.0.0

[caleblloyd]

We are trying to upgrade our MySQL provider to 2.0.0. Our Discriminator Test that tests TPH Inheritance is resulting in this NullReferenceException

Here is the Discriminator Test - read it for a good laugh, I'm put some time into making sure the Hierarchies were fun 😄

Here is the Query that is causing the Exception:

				var teachers = await db.People.OfType<PersonTeacher>()
					.Where(m => m.Id >= _fixture.LowestTeacherId && m.Id <= _fixture.HighestTeacherId)
					.OrderBy(m => m.Id)
					.Include(m => m.Family)
					.ThenInclude(m => m.Members)
					.Include(m => m.Students)
					.ThenInclude(m => m.Family)
					.ThenInclude(m => m.Members)
					.ToListAsync();

Here is the Exception:

Failed   Pomelo.EntityFrameworkCore.MySql.PerfTests.Tests.Models.DiscriminatorTest.TestDiscriminator
Error Message:
 System.NullReferenceException : Object reference not set to an instance of an object.
Stack Trace:
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler.IncludeLoadTreeNodeBase.<_AwaitMany>d__8.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler.<_IncludeAsync>d__18`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.TaskLiftingExpressionVisitor.<_ExecuteAsync>d__8`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Query.EntityQueryModelVisitor.AsyncSelectEnumerable`2.AsyncSelectEnumerator.<MoveNext>d__3.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at System.Linq.AsyncEnumerable.SelectEnumerableAsyncIterator`2.<MoveNextCore>d__7.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at System.Linq.AsyncEnumerable.AsyncIterator`1.<MoveNext>d__10.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Query.Internal.AsyncLinqOperatorProvider.ExceptionInterceptor`1.EnumeratorExceptionInterceptor.<MoveNext>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at System.Linq.AsyncEnumerable.<Aggregate_>d__6`3.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Pomelo.EntityFrameworkCore.MySql.PerfTests.Tests.Models.DiscriminatorTest.<TestDiscriminator>d__2.MoveNext() in /home/caleb/Projects/caleb/Pomelo.EntityFrameworkCore.Mysql/test/EFCore.MySql.FunctionalTests/Tests/Models/DiscriminatorTest.cs:line 96
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

[caleblloyd]

The NullReferenceException appears to be triggered by some EF internal logic in the ThenInclude statements

[maumar]

Still repros in the current bits, and only in async. Full repro:

        [Fact]
        public virtual void Repro8693()
        {
            using (var ctx = new MyContext9038())
            {
                ctx.Database.EnsureDeleted();
                ctx.Database.EnsureCreated();

                var famalies = new List<PersonFamily>
                {
                    new PersonFamily
                    {
                        LastName = "Garrison",
                    },
                    new PersonFamily
                    {
                        LastName = "Cartman",
                    },
                    new PersonFamily
                    {
                        LastName = "McCormick",
                    },
                    new PersonFamily
                    {
                        LastName = "Broflovski",
                    },
                    new PersonFamily
                    {
                        LastName = "Marsh",
                    },
                };
                var teachers = new List<PersonTeacher>
                {
                    new PersonTeacher {Name = "Ms. Frizzle"},
                    new PersonTeacher {Name = "Mr. Garrison", Family = famalies[0]},
                    new PersonTeacher {Name = "Mr. Hat", Family = famalies[0]},
                };
                var students = new List<PersonKid>
                {
                    new PersonKid {Name = "Arnold", Grade = 2, Teacher = teachers[0]},
                    new PersonKid {Name = "Phoebe", Grade = 2, Teacher = teachers[0]},
                    new PersonKid {Name = "Wanda", Grade = 2, Teacher = teachers[0]},

                    new PersonKid {Name = "Eric", Grade = 4, Teacher = teachers[1], Family = famalies[1]},
                    new PersonKid {Name = "Kenny", Grade = 4, Teacher = teachers[1], Family = famalies[2]},
                    new PersonKid {Name = "Kyle", Grade = 4, Teacher = teachers[1], Family = famalies[3]},
                    new PersonKid {Name = "Stan", Grade = 4, Teacher = teachers[1], Family = famalies[4]},
                };

                ctx.People.AddRange(teachers);
                ctx.People.AddRange(students);
                ctx.SaveChanges();
            }

            using (var ctx = new MyContext9038())
            {
                var teachersTask = ctx.People.OfType<PersonTeacher>()
                    .Include(m => m.Students)
                    .ThenInclude(m => m.Family)
                    .ThenInclude(m => m.Members)
                    .ToListAsync();

                teachersTask.Wait();
            }
        }

        public abstract class Person
        {
            public int Id { get; set; }

            public string Name { get; set; }

            public int? TeacherId { get; set; }

            public PersonFamily Family { get; set; }
        }

        public class PersonKid : Person
        {
            public int Grade { get; set; }

            public PersonTeacher Teacher { get; set; }
        }

        public class PersonTeacher : Person
        {
            public ICollection<PersonKid> Students { get; set; }
        }

        public class PersonFamily
        {
            public int Id { get; set; }

            public string LastName { get; set; }

            public ICollection<Person> Members { get; set; }
        }

        public class MyContext9038 : DbContext
        {
            public DbSet<Person> People { get; set; }

            public DbSet<PersonFamily> Families { get; set; }


            protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
            {
                optionsBuilder.UseSqlServer(@"Server=.;Database=Repro9038;Trusted_Connection=True;MultipleActiveResultSets=True");
            }

            protected override void OnModelCreating(ModelBuilder modelBuilder)
            {
                modelBuilder.Entity<PersonTeacher>().HasBaseType<Person>();
                modelBuilder.Entity<PersonKid>().HasBaseType<Person>();
                modelBuilder.Entity<PersonFamily>();

                modelBuilder.Entity<PersonKid>(entity =>
                    {
                        entity.Property("Discriminator")
                            .HasMaxLength(63);
                        entity.HasIndex("Discriminator");

                        entity.HasOne(m => m.Teacher)
                            .WithMany(m => m.Students)
                            .HasForeignKey(m => m.TeacherId)
                            .HasPrincipalKey(m => m.Id)
                            .OnDelete(DeleteBehavior.Restrict);
                    });
            }
        }

[caleblloyd]

@ajcvickers this will create bugs for anyone using TPH Inheritance with ThenInclude and retrieving via Async methods when upgrading to 2.0.0. Should it be higher priority?

[ajcvickers]

@caleblloyd We will discuss again, but at this point we have essentially no time left to get anything into 2.0 unless it is a ship-stopper type bug.
@divega Thoughts?

[divega]

@ajcvickers although it doesn't seem a ship stopper, it seems pretty bad. Can we have someone from the query team try to understand the fix?

[ajcvickers]

@maumar @smitpatel Since @anpete is out, can one of you spend some time today doing some more investigation? In particular:

• What scenarios does it repro--is it whenever two collections are included, or is it narrower than that?
• Any ideas on what the fix would look like?

[smitpatel]

Assigning to @maumar as he has done some investigation on issue already.
Assign me back if you want me to fix this.

[ajcvickers]

@maumar Also, adding to my list above, what workarounds could people use if they hit this?

[maumar]

This happens for two-level navigation include, when the first level is empty optional navigation (i.e. the included value is null). We then try to access the null value to include the collection and hence the exception. We are probably missing a null safeguard somewhere - investigating further.

Workarounds:

• use sync:

ctx.People.OfType<PersonTeacher>()
	.Include(m => m.Students)
	.ThenInclude(m => m.Family)
	.ThenInclude(m => m.Members)
	.ToList()

break query into multiple includes:

var teachersTask = ctx.People.OfType<PersonTeacher>()
	.Include(m => m.Students)
	.ThenInclude(m => m.Family).ToListAsync();

var familiesTask = ctx.Families.Include(f => f.Members).ToArrayAsync();

teachersTask.Wait();
familiesTask.Wait();

var teachers = teachersTask.Result;

filter out elements that contain null navigation:

var kidsTask1 = ctx.People.OfType<PersonKid>()
	.Where(k => k.Family != null)
	.Include(m => m.Family)
	.ThenInclude(m => m.Members)
	.ToListAsync();

var kidsTask2 = ctx.People.OfType<PersonKid>()
	.Where(k => k.Family == null)
	.ToListAsync();

kidsTask1.Wait();
kidsTask2.Wait();

var kids = kidsTask1.Result.Concat(kidsTask2.Result);

[DSilence]

This is something that we've immediately noticed after updating. A lot of our queries broke, since we're using async everywhere. Kinda frustrating that this hasn't been fixed before stable release.

[ajcvickers]

@maumar Do you think this should be considered for patch?

[maumar]

I would say so, given that this used to work before 2.0 (which we didn't realize)

[danielpmo1371]

I have made managed to use the workaround suggested by @maumar.
**Thank you @maumar **

@smitpatel or @AndriySvyryd I have just found:
https://github.com/aspnet/EntityFrameworkCore/wiki/getting-and-building-the-code
Is that the way to do the patch?

[AndriySvyryd]

@danielpmo1371 We'll ship 2.0.1 as a regular NuGet package, see https://docs.microsoft.com/en-us/nuget/consume-packages/reinstalling-and-updating-packages

[chmarti]

@AndriySvyryd is there any update on a timeline for the 2.0.1 NuGet package to be released? This is blocking my client from migrating to EF Core 2.0. I'm trying to decide if I need to go through and apply your workaround to all my queries, or if I can just wait for 2.0.1 to be released.

[ErikEJ]

@chmarti @grokky1 I am guessing November! https://dotnet.myget.org/gallery/aspnet-2-0-2-october2017-patch

[Eilon]

BTW that feed is not the correct feed, so please be careful using it because it's a fairly random set of packages on there (many of which are not meant for public consumption). We should have a publicly-consumable feed later this week. I'll update all "patch" bug issues when that feed is ready.

[ErikEJ]

@Eilon Looking forward to that - finally!

[Eilon]

Hi, we have a public test feed that you can use to try out the ASP.NET/EF Core 2.0.3 patch!

To try out the pre-release patch, please refer to the following guide:

• ASP.NET/EF Core 2.0.3 patch pre-release test feed aspnet/Announcements#274 (info)
• ASP.NET/EF Core 2.0.3 patch pre-release test feed - discussion aspnetcore#2254 (discussions and questions)

We are looking for feedback on this patch. We'd like to know if you have any issues with this patch by updating your apps and libraries to the latest packages and seeing if it fixes the issues you've had, or if it introduces any new issues. If you have any issues or questions, please reply on this issue to let us know as soon as possible.

Thanks,
Eilon

[dazinator]

Just hit this. Going to try updating to the patch from the patch preview feed.. fingers crossed.

[dazinator]

Yep, patch version fixed it.

[Eilon]

@dazinator thanks for confirming!

[deepak-khopade-isdr]

Upgrading from .NET Core 1.x to 2.0.3 and getting this error on Ubuntu 16.04; however, as workarounds suggested, tried using Sync and its working fine, however, would still wait for async to work before we really count on .NET Core 2.x.

[GermanKuber-zz]

Hello, guys. Any news about a fix for this?

[YaredDejene]

This error is fixed in EF Core v2.0.1.

[p2atran]

I'm still seeing this issue with 2.0.3. Using sync or async still reproduces the exception each time. The only difference I have is that I have an annotation on the property that is null

      [ForeignKey("TeamId")]
      public Blah myProp { get; set; }

myProp is null even when I use .Include()

@ajcvickers Is there a known bug where using both annotations and fluent api in .NET Core 2 causes the same NullReferenceException?

[maumar]

@p2atran this issue was specific to async path, if you see errors for sync then it must be a different problem. Can you file a new bug with the full code listing that reproduces the issue? We will investigate accordingly.

[p2atran]

@maumar

It looks like this thread is the same issue:
#11341

but it looks like you guys are saying it wasn't supposed to work and accidentally worked in core 1