MySQL Bugs: #55558: Internal datareader is not always correctly closed when exception occurs

Bug #55558	Internal datareader is not always correctly closed when exception occurs
Submitted:	26 Jul 2010 13:39	Modified:	29 Jul 2010 14:03
Reporter:	Pavel Bazanov	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / NET	Severity:	S2 (Serious)
Version:	6.2.3	OS:	Windows
Assigned to:	Vladislav Vaintroub	CPU Architecture:	Any
Tags:	close, DataReader, exception, MySqlDataReader, There is already an open DataReader associated with this Connection which must b

Description:
Hello,
Sometimes, after exception, internal datareader (MySqlCommand.Connection.Reader) is not properly closed (it's not set to null). So if we try to execute another query on that command object after exception - we will get "There is already an open DataReader associated with this Connection which must be closed first. " exception.

Here is what I see in my logs:

First, strange exception occurs (who know possible reasons of that exception please answer here: http://forums.mysql.com/read.php?38,377861,377861#msg-377861 ):

Exception type: System.IO.IOException
Exception message: Unable to write data to the transport connection: An existing connection was forcibly closed by the remote host.

Inner exception type: System.Net.Sockets.SocketException
Inner exception message: An existing connection was forcibly closed by the remote host

Stack trace:
   at MyNetworkStream.HandleOrRethrowException(Exception e)
   at MyNetworkStream.Write(Byte[] buffer, Int32 offset, Int32 count)
   at MySql.Data.MySqlClient.TimedStream.Write(Byte[] buffer, Int32 offset, Int32 count)
   at MySql.Data.MySqlClient.MySqlStream.SendPacket(MySqlPacket packet)
   at MySql.Data.MySqlClient.NativeDriver.ExecutePacket(MySqlPacket packetToExecute)
   at MySql.Data.MySqlClient.NativeDriver.SendQuery(MySqlPacket queryPacket)
   at MySql.Data.MySqlClient.Driver.SendQuery(MySqlPacket p)
   at MySql.Data.MySqlClient.Statement.ExecuteNext()
   at MySql.Data.MySqlClient.PreparableStatement.ExecuteNext()
   at MySql.Data.MySqlClient.PreparableStatement.Execute()
   at MySql.Data.MySqlClient.MySqlCommand.ExecuteReader(CommandBehavior behavior)
   at MySql.Data.MySqlClient.MySqlCommand.ExecuteNonQuery()
   at RA.WinForms.CreateBoxForm.SaveToDB(MySqlCommand command) in C:\VCProjects\Alex\RA\RA\WinForms\CreateBoxForm.cs:line 236

After that user tries to repeat the operation and gets another exception:

Exception type: MySql.Data.MySqlClient.MySqlException
Exception message: There is already an open DataReader associated with this Connection which must be closed first.

Stack trace:
   at MySql.Data.MySqlClient.MySqlCommand.CheckState()
   at MySql.Data.MySqlClient.MySqlCommand.ExecuteReader(CommandBehavior behavior)
   at MySql.Data.MySqlClient.MySqlCommand.ExecuteNonQuery()
   at RA.WinForms.CreateBoxForm.SaveToDB(MySqlCommand command) in C:\VCProjects\Alex\RA\RA\WinForms\CreateBoxForm.cs:line 236

I already reported this bug in the following bug report: http://bugs.mysql.com/bug.php?id=52408 , but this problem was not fixed.

How to repeat:
Here is my pseudo-code:

try
{
  command.CommandText = "...";
  command.ExecuteNonQuery(); // let's say we get exception here deep down in MySqlStream.SendPacket() method
}
catch(...)
{
  ...
}

command.CommandText = "...";
command.ExecuteNonQuery(); // there is already an open DataReader ...

Suggested fix:
I think you need to look at the MySqlDataReader.Close() method and change it to make reader always close properly and set connection.Reader to null.

Hi Pavel and thanks for your report.

Can the situation be remedied by specifying *using* keyword?

Pavel, but setting datareader to null would not help you much, right? 

The connection is hosed, server closed the socket, you cannot do anything more with the connection. Assume, we'll set datareader to null (should not be too difficult), then you'll get the same IO exception from the second comand (within catch) as from the first, correct?

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/114368

875 Vladislav Vaintroub	2010-07-26
      Bug#55558 : ensure that connection.Reader is always reset to null  in DataReader.Close(), also in case of
      exceptions.
      
      Aso,eat IOExceptions in Close() the same way as we handlle IOExceptions wrapped into MySqlException

pushed to 6.2.4, 6.3.2

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/114370

876 Vladislav Vaintroub	2010-07-26
      Fix inadverent change in the last commit (Bug#55558)

Hi Tonci and Vlad,
Thank you for the quick reaction.

Unfortunately, it seems that I was wrong when I supposed that you need to look at DataReader.Close() method.
The problem is that DataReader.Close() method is not even called!

Let's look at the Command.ExecuteNonQuery() method:

public override int ExecuteNonQuery()
{
  MySqlDataReader reader = null;
  using (reader = ExecuteReader())
  { 
  }
  return reader.RecordsAffected;
}

using statement makes programmers think that datareader will always be disposed. But if an exception occurs inside ExecuteReader, then internally created reader is not returned and so will not be disposed.

(Also, you access reader.RecordsAffected property on disposed reader, that's not good..)

So we need to clean up inside ExecuteReader() method if exception occurs.
I would add a boolean flag executedSuccesfully and if it's false I would dispose reader and set connection.Reader = null inside finally block.

So, I tried to fix it like this:

=== изменён файл MySql.Data/Provider/Source/command.cs
--- MySql.Data/Provider/Source/command.cs	2010-07-01 22:51:18 +0000
+++ MySql.Data/Provider/Source/command.cs	2010-07-27 22:12:56 +0000
@@ -413,14 +413,17 @@
             HandleCommandBehaviors(behavior);
 
 			updatedRowCount = -1;
+            bool executedSuccesfully = false;
+            MySqlDataReader reader;
             try
             {
-                MySqlDataReader reader = new MySqlDataReader(this, statement, behavior);
+                reader = new MySqlDataReader(this, statement, behavior);
                 connection.Reader = reader;
                 // execute the statement
                 statement.Execute();
                 // wait for data to return
                 reader.NextResult();
+            		executedSuccesfully = true;
                 return reader;
             }
             catch (TimeoutException tex)
@@ -462,7 +465,14 @@
             }
             finally
             {
-                if (connection != null && connection.Reader == null)
+							if (!executedSuccesfully)
+							{
+								if(connection != null)
+									connection.Reader = null;
+								if(reader != null)
+									reader.Dispose();
+							}
+            		if (connection != null && connection.Reader == null)
                 {
                     // Comething want seriously wrong,  and reader would not be 
                     // able to clear timeout on closing.

But I found another problem:
To simulate real exception conditions I throw SocketException inside MySqlStream.SendPacket() method.
Then, if I try to execute another query (on the same command) after exception, I get result of a previous query:

MySqlCommand command = new MySqlCommand("", conn);
try
{
	command.CommandText = "SELECT NOW()";
	var o = command.ExecuteScalar(); // I throw SocketException in MySqlStream.SendPacket()
}
catch
{
  // swallow and try to proceed
}

command.CommandText = "SELECT VERSION()";
var oo = command.ExecuteScalar(); // I get result of a previous query - current datetime

So it seems my patch doesn't make a full clean up.. Hope you make it right..

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/114507

880 Vladislav Vaintroub	2010-07-28
      Bug#55558: 
      
      Always set internal datareader to null  if ExecuteReader fails
      to prevent subsequent "data reader is already open exception".
      
      Also, improve exception handling if network writes fail
      (can be reproduced when the database goes down
      just before ExecuteReader):
      
      * catch IOException in ExecuteReader
       in the catch block: 
      * abort  connection (close, without returning to pool)
      * wrap IOException into MySqlException 
      * rethrow

Thanks Pavel, it was helpful.

I fixed using disposed reader in the other commit (well spotted), and used your suggestions here to improve on the fix. 

Connection should be closed if errors like this (IO errors) occur.

Here however, IO exception was not dealt with, and it went uncaught and unhandled to the user. I fixed that in the patch.

Such an error, that happens on network "write"  rather than "read" is not so easy to reproduce, one way to do that is to shutdown or kill the database after connection open and before command.ExecuteXXX.

Hi Vlad,

in Command.ExecuteReader() method:

catch (MySqlException ex)
{
  connection.Reader = null;
  if (ex.InnerException is TimeoutException)
    throw ex; // already handled

As you can see, connection.Reader is set to null. In this case reader will not be disposed in the finally block. So, I would remove setting connection.Reader to null from the catch(MySqlException ex) block.

And maybe it's better to use throw, instead of throw ex?

Also, I am still worried about leaving things in inconsistent state. As I wrote in my previous message, if I emulate SocketException from MySqlStream.SendPacket then the next query returns results of the previous query. I know this is a generic situation created by me, but this is very dangerous (I mean getting wrong results is worse than not getting results at all), so we need to think if such a situation can occur in real practice. Vlad, do you understand the problem I am describing? Because I am really worried..

PS. Guys, what GUI-client for Bazaar do you use? I use Tortoise Bazaar and I'm not really happy with it :(

Pavel,
you do not need to be worries about your socket exception. Stream classes do not throw socket exceptions, they throw IOExceptions (perhaps with socket exceptions as InnerException), which are handled. Except IO exceptions, there is special TimeoutException we throw and handle ourselves. And there are MySqlExceptions, which are either errors returned by serverm , or wrapped IO exceptions. And, there is exotic case for ThreadAbortException.

All of them are handled. IOException you'll get in real life will result in closing the connection, and this is likely what you want.

I currently hesitate to add common handler for Exception to catch non-real-life scenarios (like SocketException), unless it is proven we can get it somehow.

Your point about nulling Reader in MySqlException case is valid and thanks for advice. Same about "throw ex;"

PS. For bzr, I use command line and mix of standard bzr (bzr branch, pull. merge) and Qbzr commands (bzr qlog, bzr qannotate, bzr qdiff etc)

Hello, Vlad,
Thank you for the fix.

I want to say a couple of words about revision 907.
Let's look at the ExecuteNonQuery() method:

public override int ExecuteNonQuery()
{
        using (MySqlDataReader reader = ExecuteReader())
        {
            reader.Close();
            return reader.RecordsAffected;
        }
}

reader.Close() can throw NullReferenceException, because ExecuteReader() can return null (see catch(MySqlException) block).

Also, I don't think it's a good idea to access RecordsAffected property after the reader is closed. I try not to access any methods of an object after it is closed or disposed..

Pavel, thanks for looking.
As a matter of fact you RecordsAffected are only reliable after the Reader is closed. It is not disposed, so the object is valid.

As for NullReferenceException, well, things can happen :)

...but for things not to happen, I added check for reader == null to ExecuteNonQuery() now.

Thanks Pavel, again well spotted.

Vlad,
Are you sure it is a good idea to return 0 from ExecuteScalar() when MySqlException occurs? It hides exceptions and returns 0, meaning "everything is ok, query returned value 0". But that's incorrect. I don't think that swallowing exceptions is ok. Maybe we always need to rethrow exceptions from ExecuteReader()?

Pavel, no I'm not very sure about ExecuteScalar (but last push did not touch that code...)

returning 0 from ExecuteNonQuery() (that was the last push) does not sound too bad , and certainly is accurate in many cases. (We would assume the query was an interrupted update, so 0 rows were affected)

Generally, perhaps it might be worth to rethink how we handle interrupted (killed) queries. With some exceptions (i.e timeout handling where we use kill ourselves), maybe it is fine to expose it as MySQLException to the caller.

I am sorry, I thought I was looking at the ExecuteScalar() method when I was actually looking at ExecuteNonQuery() =)

Anyway, I still think it would be better to rethrow MySqlException from ExecuteReader() instead of swallowing it...
I am sorry if I am disturbing you and want too much :) Just sharing my thoughts..

An entry has been added to the 6.2.4 and 6.3.2 changelogs:

After an exception, the internal datareader, MySqlCommand.Connection.Reader, was not properly closed (it was not set to null). If another query was subsequently executed on that command object an exception was generated with the message “There is already an open DataReader associated with this Connection which must be closed first.”