Efcore: `System.InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first.` when explicitly loading navigation property in DbSet<> foreach loop

[lauxjpn]

An issue got reported on our repo (https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/issues/1032), that turned out to be an implementation issue in EF Core.

The following code will work in Pomelo and SqlServer without issue:

```c#
using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pomelo.EntityFrameworkCore.MySql.Storage;

namespace IssueConsoleTemplate
{
public class Blog
{
public int BlogId { get; set; }
public string Url { get; set; }

    public virtual List<Post> Posts { get; } = new List<Post>();
}

public class Post
{
    public int PostId { get; set; }
    public string Title { get; set; }
    public string Content { get; set; }

    public int BlogId { get; set; }
    public virtual Blog Blog { get; set; }
}

public class BloggingContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }
    public DbSet<Post> Posts { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder
            //.UseMySql("server=127.0.0.1;port=3306;user=root;password=;database=Issue1032",
            //    b => b.ServerVersion(new ServerVersion("8.0.18-mysql")))
            .UseSqlServer(@"Data Source=.\MSSQL14;Integrated Security=SSPI;Initial Catalog=Issue1032")
            .UseLoggerFactory(LoggerFactory.Create(b => b
                .AddConsole()
                .AddFilter(level => level >= LogLevel.Information)))
            .EnableSensitiveDataLogging()
            .EnableDetailedErrors();
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Blog>()
            .HasData(
                new Blog {BlogId = 2, Url = "https://devblogs.microsoft.com/dotnet"}
            );

        modelBuilder.Entity<Post>()
            .HasData(
                new Post {PostId = 3, Title = "2nd Post", Content = "I am an expert now!", BlogId = 2},
                new Post {PostId = 4, Title = "Hello World", Content = "I wrote an app using EF Core!", BlogId = 2}
            );
    }
}

internal class Program
{
    private static void Main()
    {
        using (var db = new BloggingContext())
        {
            db.Database.EnsureDeleted();
            db.Database.EnsureCreated();

            var blogs = db.Blogs.ToList(); // <!-- Works
            // var blogs = db.Blogs; // <!-- Fails

            foreach (var blog in blogs)
            {
                db.Entry(blog)
                    .Collection(b => b.Posts)
                    .Load();
            }
        }
    }
}

}

When you change `var blogs = db.Blogs.ToList();` to `var blogs = db.Blogs;`, you will get the following exception, because the `DataReader` from the `blogs` query is naturally still open while being iterated over by the foreach loop, when hitting the instructions to explicitly load the current blog's posts:

#### SqlServer

System.InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first.
at at Microsoft.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlCommand command)
at at Microsoft.Data.SqlClient.SqlConnection.ValidateConnectionForExecute(String method, SqlCommand command)
at at Microsoft.Data.SqlClient.SqlCommand.ValidateCommand(Boolean isAsync, String method)
at at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String method) at at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method) at at Microsoft.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior) at at Microsoft.Data.SqlClient.SqlCommand.ExecuteDbDataReader(CommandBehavior behavior) at at System.Data.Common.DbCommand.ExecuteReader() at at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReader(RelationalCommandParameterObject parameterObject) at at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable1.Enumerator.InitializeReader(DbContext _, Boolean result)
at at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.ExecuteTState,TResult
at at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable1.Enumerator.MoveNext() at at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.Load[TSource](IQueryable1 source)
at at Microsoft.EntityFrameworkCore.Internal.EntityFinder`1.Load(INavigation navigation, InternalEntityEntry entry)
at at Microsoft.EntityFrameworkCore.ChangeTracking.NavigationEntry.Load()
at at Microsoft.EntityFrameworkCore.ChangeTracking.CollectionEntry.Load()
at IssueConsoleTemplate.Program.Main() in E:\Sources\Issue1032\IssueConsoleTemplate\Program.cs:74

#### Pomelo

System.InvalidOperationException: Cannot set MySqlCommand.CommandText when there is an open DataReader for this command; it must be closed first.
at MySql.Data.MySqlClient.MySqlCommand.set_CommandText(String value) in E:\Sources\MySqlConnector\src\MySqlConnector\MySql.Data.MySqlClient\MySqlCommand.cs:150
at at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.CreateCommand(RelationalCommandParameterObject parameterObject, Guid commandId, DbCommandMethod commandMethod)
at at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReader(RelationalCommandParameterObject parameterObject)
at at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable1.Enumerator.InitializeReader(DbContext _, Boolean result) at Pomelo.EntityFrameworkCore.MySql.Storage.Internal.MySqlExecutionStrategy.Execute[TState,TResult](TState state, Func3 operation, Func3 verifySucceeded) in E:\Sources\Pomelo.EntityFrameworkCore.MySql\src\EFCore.MySql\Storage\Internal\MySqlExecutionStrategy.cs:48 at at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable1.Enumerator.MoveNext()
at at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.Load[TSource](IQueryable1 source) at at Microsoft.EntityFrameworkCore.Internal.EntityFinder1.Load(INavigation navigation, InternalEntityEntry entry)
at at Microsoft.EntityFrameworkCore.ChangeTracking.NavigationEntry.Load()
at at Microsoft.EntityFrameworkCore.ChangeTracking.CollectionEntry.Load()
at IssueConsoleTemplate.Program.Main() in E:\Sources\Issue1032\IssueConsoleTemplate\Program.cs:74
```

Further technical details

EF Core version: 3.1.2
Database provider: any
Target framework: .NET Core 3.1
Operating system: Windows
IDE: Rider 2019.3.3

[smitpatel]

SqlServer requires MARS to run multiple queries enumerating at the same time. If database does not support it, at least one of the queries has to buffer the data and close the reader before other query sends the command. We used to do this internally in 2.2-. We stopped doing that in 3.0+. Doing buffering implicitly can mask the fact high memory is being used. And user would be more aware which query results are smaller to do buffering on that query themselves.

@roji - What do you do for this kind of scenarios since postgre it is always not supported. (Unlike SqlServer which shows both behavior)

[ajcvickers]

@smitpatel I'm not convinced that we shouldn't bring back the buffering behavior, possibly as an option if we feel that buffering by default is a bad idea. Note, however, that we have to buffer by default when using Azure connection resiliency, and I don't remember seeing complaints about it.

[roji]

@smitpatel I just tell users to add ToList :) FWIW nobody has complained about it so far once made aware of it, I think it's a good idea to keep it explicit. It's not always immediately apparent where multiple round-trips happen (and therefore also buffering), so I think the ToList helps making things clear. But you already know I'm always on the explicit side of things :)

[mguinness]

I just tell users to add ToList :) FWIW nobody has complained about it so far

That's fine, but it would be nice if a another exception is raised beforehand with perhaps a link via http://go.microsoft.com/fwlink to explain the issue in more detail. At a minimum this restriction should be noted in Loading Related Data.

[smitpatel]

Though this is not just loading related data. Any time you run multiple queries in parallel like that when underlying database don't support it, it is raised.

As for throwing a different message. For SqlServer, we don't know till we run it and we don't want to be parsing error message to throw a different exception. More-or-less the message is decent enough.

Looking at stacktrace, perhaps, MySql provider can check if datareader is closed in RelationalCommand.CreateCommand before setting command text. That could throw better error message in bad case.

[ajcvickers]

We discussed this in triage and decided for now not to do anything specifically for this issue, although we could revisit that based on more feedback.

@mguinness We have discussed this several times, but as @smitpatel alluded to, it's not easy since it is a provider-specific exception which would need to be handled by non-provider-specific code. So this would require a mechanism for EF to call back into the provider to handle creating a different message. This is all doable, we're just not convinced that the cost is worth the value.

[lauxjpn]

I handled this like @roji does, just by educating users about how to deal with this issue.

But I do agree with @mguinness, that the docs should be more explicit about this behavior, because this is totally legit code that is being run and MARS is not necessarily a commonly supported feature.

As for @smitpatel's suggestion: We would definitely be able to throw a more specific error message.

The question for me is more of the nature @ajcvickers stated, whether EF Core wants to handle this type of cases for .NET 5 in some way, or if this should just be handled through the docs and provider specific implementations.

Personally, I see this as a minor issue and nothing important that needs to be addressed by implementation changes. But having this documented (in more than this GitHub issue) would probably be good, because especially EF Core rookies tend to stumble over this behavior and it does not seem to be common knowledge, that an active enumerator will keep a DataReader open (and the fact that this will make further queries fail).

[smitpatel]

I would be on the side of provider specific since exception message would be different for each provider. Also in case or MySql, it can be checked before executing command, for SqlServer, command needs to be executed.
Also,

• Sqlite - Never has this issue
• SqlServer - Errors when MARS is not on. Throws exception only when executing command.
• npgsql - Never supports. I am not sure when we can figure out if it is failing. @roji can comment more on that.
• MySql - Client fails to create DbCommand. Executing the command is not required.

Due to each provider being so different. there is not much value in trying to abstract it over in relational codebase. Provider specific implementation would give opportunity to do targeted check since path could be perf sensitive.

[roji]

@smitpatel Npgsql does have an internal detection mechanism for this, and throws a special NpgsqlOperationInProgressException, referencing the command already in progress (so you can see its SQL). It would be trivial to recognize this exception in the EF Core provider and raise something else, but I think I'll wait to see more users hit the issue first, to see that it's worth it :)

[ajcvickers]

Discussed again in triage and we're going to go with basically what @smitpatel wrote above. That is:

• No automatic buffering in this case
  
  
  
  • It has no real advantage over explicit buffering with ToList or similar
  
  
• No exception interception mechanism
  
  
  
  • Providers that can throw a better message should do so
  
  
  • For SQL Server we don't plan to try to change the message