MySQL Bugs: #89159: ResetReader: MySqlDataReader cannot outlive parent MySqlCommand since 6.10

Bug #89159	ResetReader: MySqlDataReader cannot outlive parent MySqlCommand since 6.10
Submitted:	9 Jan 2018 12:41	Modified:	7 Feb 2020 16:07
Reporter:	Timo van Zijll Langhout	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / NET	Severity:	S2 (Serious)
Version:	6.10, 8.0.18	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	MySqlDataReader MySqlCommand ResetReader Dispose

Description:
Summary:

A change in 6.10 now causes MySqlCommand.Dispose() to call its ResetReader() method. In effect, we can no longer have a method that simple returns a DbDataReader while abstracting away all the DbConnection and DbCommand details, because such a method needs to dispose the DbCommand it has created, without closing the DbDataReader it is returning.

Details:

In most cases, we create and dispose in the following order:

- Create DbConnection
-  Create DbCommand (connection.CreateCommand())
-   Create DbDataReader (command.ExecuteReader())
-   Dispose DbDataReader
-  Dispose DbCommand
- Dispose DbConnection

However, in some cases we want to return the DbDataReader to the client code. For this purpose, we use command.ExecuteReader(CommandBehavior.CloseConnection), allowing the client to close the DbDataReader and DbConnection all at once when they are done reading.

Returning the DbCommand also would force the client code to juggle multiple disposable objects. Instead, it is common practice that the method returning the DbDataReader closes the DbCommand, as it is no longer required.

An example can be found here: https://stackoverflow.com/a/32264644/543814
As the author mentions, this sample comes directly from Microsoft. The original can be found here: https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlcommand(v=vs.110).aspx?f...

The code sample (also presented under "How to repeat") shows a practical way to abstract away the use of DbCommand and DbConnection, returning a simple DbDataReader. Indeed, it is a straightforward method to avoid endless repetition of this boilerplate code.

The fact that MySqlCommand.Dispose() now calls its ResetReader() method breaks this approach in two ways:

1. The DbDataReader is disposed and thus unusable.
2. MySqlCommand.Dispose() now actually throws an exception if the DbDataReader wasn't disposed FIRST. Specifically, Dispose() calls ResetReader(), ResetReader() calls connection.Reader.Close(), because of CommandBehavior.CloseConnection that results in connection.CloseFully(), which nulls out connection.driver, and finally ResetReader() proceeds to null out connection.Reader, whose setter tries to read driver.reader, resulting in a NullReferenceException. (Why does this not occur if the reader is disposed first? Because ResetReader() short-circuits if connection.Reader is already null.)

How to repeat:
Run this simple method, replacing the Sql types by the corresponding MySql types.

When the code exits the using() and disposes cmd, the described exception occurs, plus the reader gets disposed.

  public static SqlDataReader ExecuteReader(String connectionString, String commandText,
      CommandType commandType, params SqlParameter[] parameters) {
     SqlConnection conn = new SqlConnection(connectionString);

     using (SqlCommand cmd = new SqlCommand(commandText, conn)) {
        cmd.CommandType = commandType;
        cmd.Parameters.AddRange(parameters);

        conn.Open();
        // When using CommandBehavior.CloseConnection, the connection will be closed when the 
        // IDataReader is closed.
        SqlDataReader reader = cmd.ExecuteReader(CommandBehavior.CloseConnection);

        return reader;
     }
  }

Suggested fix:
MySqlCommand.Dispose() must avoid calling ResetReader(). This is also precisely the change that caused the problem.

I do not know the reason that it was added, or if anything might now remain undisposed if we remove it. If there is an essential reason for it to be there, perhaps a condition can be introduced that only makes the call if it is unavoidable. However, as per the description, the command closing the reader seems fundamentally less-than-ideal.

This bug was also reported on Stack Overflow here: https://stackoverflow.com/q/47779128

Hello Timo van,

Thank you for the bug report.
Verified based on internal discussion with dev's.

Thanks,
Chiranjeevi.

This bug further affects following scenario:

- Open a command, do stuff with it but do not dispose of it yet.
- Open another command and open a reader from it.
- Dispose of the first command.

This result in the first command closing the reader of the second command!

Although the root cause is the same, it looks worth a dedicated bug report.

Concrete case where this was discovered: polymorphic query with NHibernate returned as an Enumerable with deferred execution of reads on DataReader. Such a query results in as many commands as there are implementations of the queried entity, each command opening a reader with a safeguard in NHibernate for databases not supporting multiple opened reader ensuring previously opened reader are read into memory and closed prior to the new reader opening. But the command of the reader transfered into memory is not disposed of at that moment, but only a bit later, while another reader has already been opened.

Previous comment now reported as #90086

I would say, for me(us) this is more than S3 (Non-critical). When company is actively seeking support for MySql and already has abstraction layer that supports Sql Server and Oracle, this becomes a show stopper.

A code that is DB-Agnostic and has supported Oracle.DataAccess/Oracle.ManagedDataAccess/SqlClient/OleDb with no issues:

Dim conn As IDbConnection = NativeConnection.GetConnection(connInfo)
Using cmd As IDbCommand = NativeCommand.GetCommandObject(conn)
    ' +++ Do more stuff to connection here
    Dim reader As IDataReader = cmd.ExecuteReader(CommandBehavior.CloseConnection)
    ' +++ Do more stuff to connection here
    Return reader
End Using

can't support MySqlClient because in MySqlCommand.Dispose() MySqlReader being closed. Other providers call Dispose() in Close(). In fact, CommandBehavior.CloseConnection should be the indicator that MySqlReader should close the connection and MySqlcommand should just fade away gracefully.

I believe, this bug should be pushed as higher priority. I think, MySql Devs should disassemble major providers from Microsoft and Oracle and follow the established pattern.  We want to pay MySql royalties but we don't want to spend $$ to code for it separately. We have huge system and a lot of SQL. This is huge for us and any company that has data layer that supports multiple DB Engines. Thanks

Changing severity to serious, notice that severity is defined when reporter filed the bug.

@Miguel Solorzano
I appreciate you expediting this and I understood from the beginning that originator set the priority. Thank you

This has been reported as a bug against Dapper (since it uses the command disposal order described in the OP): https://github.com/StackExchange/Dapper/issues/1101

However, the problem is not really in Dapper; it's triggered by this same underlying issue.

I'd be interested to know if there are any plans to fix this.

Bug #97242 marked as duplicate of this one.

Posted by developer:
 
Fixed as of the upcoming MySQL Connector/NET 8.0.20 release, and here's the proposed changelog entry from the documentation team:

Access to the MySqlDataReader object was restricted when the parent MySqlCommand
object was closed. This fix modifies MySqlCommand.Dispose() to no longer
call the ResetReader method.

Thank you for the bug report.