Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullReferenceException in MySqlConnector.MySqlDataReader.ActivateResultSet #1459

Closed
feiazifeiazi opened this issue Mar 6, 2024 · 30 comments
Closed
Assignees
Labels

Comments

@feiazifeiazi
Copy link

feiazifeiazi commented Mar 6, 2024

An optimization introduced in v2.3.0 started recycling MySqlDataReader instances: #1277.

If client code held onto the MySqlDataReader and disposed it after the associated MySqlConnection had been returned to the connection pool, this could cause that connection to be disposed while it was being used by another thread, leading to unpredictable NullReferenceExceptions.

While many access patterns didn't trigger this bug (e.g., no problems have been reported by users of Pomelo.EntityFrameworkCore.MySql), using CommandBehavior.CloseConnection or Dapper's QueryAsync<T> made it more likely to happen.

Original bug report below.


Software versions
MySqlConnector version: 2.3.5
Server type (MySQL, MariaDB, Aurora, etc.) and version: 8.0.25
.NET version: net7.0
Dapper version: 2.1.28

Describe the bug
NullReferenceException

Exception

[Error Message]=Object reference not set to an instance of an object.
[Exception Type]=System.NullReferenceException
[Source]=MySqlConnector
[TargetSite]=Void ActivateResultSet(System.Threading.CancellationToken)
[Stack Trace]=   at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 133
   at MySqlConnector.MySqlDataReader.InitAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 505
   at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 78
   at MySqlConnector.MySqlCommand.ExecuteNonQueryAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 317
   at MySqlConnector.MySqlCommand.ExecuteNonQuery() in /_/src/MySqlConnector/MySqlCommand.cs:line 108
   at Dapper.SqlMapper.ExecuteCommand(IDbConnection cnn, CommandDefinition& command, Action`2 paramReader) in /_/Dapper/SqlMapper.cs:line 2975
   at Dapper.SqlMapper.ExecuteImpl(IDbConnection cnn, CommandDefinition& command) in /_/Dapper/SqlMapper.cs:line 656
   at Wwtfly.DBMonitor.DAL_Dapper.DBLinkConfigDAL.UpdateExecCount()

Code sample

-

Expected behavior

Additional context
This method is an update SQL . Its execution frequency is approximately 50 times per second.

The error randomly occurs 20 times per day.

Link to the exception line

@bgrainger
Copy link
Member

Can you provide a self-contained example that reproduces the problem?

A NullReferenceException in MySqlConnector is most typically indicative of broken internal invariants, usually caused by multithreaded misuse. See https://mysqlconnector.net/troubleshooting/connection-reuse/ and note that connections must not be accessed by multiple threads at the same time.

@bgrainger
Copy link
Member

Related: #1427 (comment)

@feiazifeiazi
Copy link
Author

Can you provide a self-contained example that reproduces the problem?

A NullReferenceException in MySqlConnector is most typically indicative of broken internal invariants, usually caused by multithreaded misuse. See https://mysqlconnector.net/troubleshooting/connection-reuse/ and note that connections must not be accessed by multiple threads at the same time.

thank you very much. I will study the code. (This is old code, converted from Oracle MySQL driver.)

@jayrowe
Copy link

jayrowe commented Mar 15, 2024

I'm seeing the same thing.

Nothing database-related changed other than updating the versions of MySqlConnector and Dapper earlier this week.

I looked for threading issues. The places where database connections are created and used are pretty minimal (abstracted into some common code). Every connection create has a using statement and there aren't any places where the connection is created and then returned to the caller.

I'm going to try rolling back to an older version to see what happens.

More Context:

  • Upgraded MySqlConnector from 2.1.11 to 2.3.5
  • Upgrade Dapper from 2.0.123 to 2.1.35

Sometimes there is a null reference during Dispose:

   at MySqlConnector.MySqlDataReader.DisposeAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 612
   at System.Data.Common.DbDataReader.Dispose(Boolean disposing)
   at MySqlConnector.MySqlDataReader.Dispose(Boolean disposing) in /_/src/MySqlConnector/MySqlDataReader.cs:line 443
   at Dapper.SqlMapper.QueryAsync[T](IDbConnection cnn, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 472

Sometimes there is an null reference during ActivateResultSet:

   at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 117
   at MySqlConnector.MySqlDataReader.InitAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 483
   at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 56
   at MySqlConnector.MySqlCommand.ExecuteReaderAsync(CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 357
   at MySqlConnector.MySqlCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 350

There is a packet out-of-order error, too:

One or more errors occurred. (Packet received out-of-order. Expected 1; got 2.)

I don't have a usable stack trace for the last one, since it is wrapped up in an AggregateException

@bgrainger
Copy link
Member

Sometimes there is a null reference during Dispose:

   at MySqlConnector.MySqlDataReader.DisposeAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 612
   at System.Data.Common.DbDataReader.Dispose(Boolean disposing)
   at MySqlConnector.MySqlDataReader.Dispose(Boolean disposing) in /_/src/MySqlConnector/MySqlDataReader.cs:line 443
   at Dapper.SqlMapper.QueryAsync[T](IDbConnection cnn, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 472

This call stack doesn't "look right". I wonder if there's an earlier exception that's being thrown, but this NullReferenceException is coming from a second exception being thrown while finally blocks are being run.

@jayrowe
Copy link

jayrowe commented Mar 19, 2024

@bgrainger

I agree, but if you look at that stack trace, you can see that it's in Dapper. Dapper is opening and closing the connections and disposing the reader for me. I'm really not managing connections other than creating them and calling Dispose or DisposeAsync. I'm dealing with the readers even less frequently.

I rolled back to 2.1.11 yesterday midday, and it hasn't happened since. It was happening quite consistently since I upgraded. The chart of error counts related to this is pretty damning so far. You can see when I upgraded and when I downgraded; usage has been consistent throughout the time period.

image

@bgrainger
Copy link
Member

I work on systems with MySqlConnector 2.3.5 deployed to production and opening about 1,000,000 database connections per hour. We have not seen widespread NullReferenceExceptions like you're reporting.

Are you able to provide a small repro that demonstrates the problem?

Or can you provide real (not abbreviated or sanitised) examples of what your calling code (with Dapper) looks like?

@jayrowe
Copy link

jayrowe commented Mar 19, 2024

I tried inducing the failure before I rolled back but was unable to do it on demand.

It's pretty bog standard. Things are abstracted for instrumentation purposes so the failing methods generally are one of these:

    protected async Task<List<TResult>> QueryAsync<TResult>(string sql, object? parameters = null, CommandFlags flags = CommandFlags.Buffered, CancellationToken cancellationToken = default)
    {
        flags |= CommandFlags.Buffered;

        using var connection = _connectionFactory.Create();

        return (List<TResult>)await connection.QueryAsync<TResult>(
            new CommandDefinition(
                sql,
                parameters,
                flags: flags,
                cancellationToken: cancellationToken));
    }

    protected async Task ExecuteAsync(string sql, object? parameters = null, CancellationToken cancellationToken = default)
    {
        using var connection = _connectionFactory.Create();

        await connection.ExecuteAsync(
            new CommandDefinition(sql, parameters, cancellationToken: cancellationToken));
    }

    protected async Task<TResult> ExecuteScalarAsync<TResult>(string sql, object? parameters = null, CancellationToken cancellationToken = default)
    {
        using var connection = _connectionFactory.Create();

        return await connection.ExecuteScalarAsync<TResult>(
            new CommandDefinition(sql, parameters, cancellationToken: cancellationToken));
    }

_connectionFactory.Create() just creates the new instance with the proper connection string as there are separate databases and users with different permissions.

I looked through the cases not using one of those methods and most follow the exact same pattern - create connection with a using statement, call Dapper. The ones that don't follow the pattern are where there is a low number of iterations calling a stored procedure to do something complicated, so the code calls OpenAsync on the connection once and leaves it open.

It's happening across different connections to different databases with different connection strings, and rolling back made the problem disappear for the last 24+ hours after having been consistent for five or so days.

I'm on .NET 8 and I'm connecting to an AWS Aurora MySQL cluster if either of those is a complicating factor.

@bgrainger
Copy link
Member

bgrainger commented Mar 19, 2024

Thanks for the update. That code looks pretty "bog standard", as you said. CommandFlags.Buffered is a little unusual (I don't think most Dapper users use it), but shouldn't cause any problems.

Does _connectionFactory.Create() also open the connection, or do you (almost always) let Dapper open and close the connection?

@jayrowe
Copy link

jayrowe commented Mar 20, 2024

It just creates the connection and returns it; it does not open it. With very few exceptions I let Dapper open and close the connection.

I'm swamped right now but I'll try to circle back when I get a chance. I can probably deploy a version to my staging environment with an instrumented build to try to catch it in action. It happens in that environment but the frequency of data updates is much lower so it isn't as consistent as it was in my production environment.

@bgrainger
Copy link
Member

I see you're passing in cancellationToken: cancellationToken. Is it possible that these exceptions are correlated with cancelled queries? Do you know how frequently cancellation is happening?

@jayrowe
Copy link

jayrowe commented Mar 20, 2024

I would expect it to be rare; the case would be a cancelled http request in a user-facing path. Most of our content is served up from OpenSearch, not from the database.

The paths throwing exceptions are in background message processing for data updates and those don't pass in any CancellationToken.

@lazorfuzz
Copy link

+1 we are seeing the exact same issue, with Dapper 2.0.90 and MySqlConnector 2.3.7, on .NET 6. Pretty standard usage, no crazy black magic with threads as far as I can tell.

@bgrainger
Copy link
Member

Is anyone who is experiencing this problem able to provide a self-contained example that reproduces the exception?

@bgrainger
Copy link
Member

Upgraded MySqlConnector from 2.1.11 to 2.3.5

One major change in 2.3.5 was #1277 which recycles MySqlDataReader instances. Having two threads use the same instance at once would be a significant bug (and misuse of the library).

at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 133

The only thing that could be null on https://github.com/mysql-net/MySqlConnector/blob/2.3.5/src/MySqlConnector/MySqlDataReader.cs#L133 is m_resultSet.ReadResultSetHeaderException but we're inside an if block that checks if it's not null.

at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 117

This line https://github.com/mysql-net/MySqlConnector/blob/2.3.5/src/MySqlConnector/MySqlDataReader.cs#L117 also can't throw, but the lines before and after also read m_resultSet.ReadResultSetHeaderException.

This value is set to null in ResultSet.Reset; if that were somehow being called on another thread, we could see this exception.

@bgrainger
Copy link
Member

Is anyone who is experiencing this problem able to provide a self-contained example that reproduces the exception?

Another thing that might help is providing trace-level logging leading up to the exception.

@Vofkin-San
Copy link

Vofkin-San commented Apr 19, 2024

Hey.
We have the same issue after upgrading MySqlConnector to 2.3.5

Context:
Upgraded MySqlConnector to 2.3.5
Upgrade Dapper to 2.1.35

The downgrade MySqlConnector to 2.3.1 does not help.
BTW, here #1427 (comment)
on MySqlConnector v2.3.1 it leads to the same behavior.

Here is a Stack trace from MySqlConnector v2.3.1:

System.NullReferenceException: Object reference not set to an instance of an object.
at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 117
at MySqlConnector.MySqlDataReader.InitAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 483
at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 56
at MySqlConnector.MySqlCommand.ExecuteReaderAsync(CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 357
at MySqlConnector.MySqlCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 350
at Datadog.Trace.ClrProfiler.CallTarget.Handlers.Continuations.TaskContinuationGenerator`4.SyncCallbackHandler.ContinuationAction(Task`1 previousTask, TTarget target, CallTargetState state)
at Dapper.SqlMapper.QueryRowAsync[T](IDbConnection cnn, Row row, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 489 

Connections are not disposed explicitly, "await using" used for that.

await using var connection = new MySqlConnection(_mySqlConnectionString);
var entities = await connection.QueryAsync<DbEntity>(cmdDef);
return entities; 

@bgrainger
Copy link
Member

I created a test program to issue a large number of simultaneous queries to MySQL Server with Dapper (using QueryAsync<T>), using combinations of the following variations:

  • mysql:8.3 Docker container and Azure Database for MySQL
  • CommandFlags.None and CommandFlags.Buffered
  • no CancellationToken and CancellationTokenSource.CancelAfter
  • net8.0 and net48
  • 10 rows of 2 columns and 1,000 rows of 6 columns
  • MaximumPoolSize of 100 and 250

After several dozen runs, I never saw a NullReferenceException thrown.

At this point I think I need someone who's experiencing the problem to provide either a repro or detailed logs.

@bgrainger
Copy link
Member

I wonder if there's an earlier exception that's being thrown, but this NullReferenceException is coming from a second exception being thrown while finally blocks are being run.

I think this is what's happening. I was able to reproduce the following two call stacks by deliberately injecting errors into the MySQL protocol being returned to MySqlConnector (and ignoring them with catch (MySqlProtocolException):

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 118
   at MySqlConnector.MySqlDataReader.InitAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 483
   at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 56
   at MySqlConnector.MySqlCommand.ExecuteReaderAsync(CommandBehavior behavior, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 357
   at MySqlConnector.MySqlCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 350
   at Dapper.SqlMapper.QueryAsync[T](IDbConnection cnn, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 434
Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at MySqlConnector.MySqlDataReader.DisposeAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 612
   at MySqlConnector.MySqlDataReader.Close() in /_/src/MySqlConnector/MySqlDataReader.cs:line 339
   at MySqlConnector.MySqlDataReader.Dispose(Boolean disposing) in /_/src/MySqlConnector/MySqlDataReader.cs:line 443
   at Dapper.SqlMapper.QueryAsync[T](IDbConnection cnn, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 472

This only happens in v2.3.0 and later, but not in v2.2.7 and earlier.

@bgrainger
Copy link
Member

I think this is what's happening. I was able to reproduce the following two call stacks by deliberately injecting errors into the MySQL protocol being returned to MySqlConnector

Further investigation seems to indicate that CommandBehavior.CloseConnection (which Dapper sets if the connection isn't already open) is the problem. I'm able to reproduce these exceptions with this sample code (without injecting protocol failures):

Task.Run(async () =>
{
    await Task.Yield();
    using var connection = new MySqlConnection(csb.ConnectionString);
    await connection.OpenAsync();
    using var command = new MySqlCommand("SELECT 1;", connection);

    using var reader = await command.ExecuteReaderAsync(CommandBehavior.CloseConnection);
    return 1;
}));

For now, I can only get a repro with my in-memory MySQL protocol simulation, not with a real MySQL Server; I'm not sure why that is.

This code looks problematic: closing the connection will return the session to the pool and allow the MySqlDataReader to be picked up by another thread; then the current thread will clear values on it:

if ((m_behavior & CommandBehavior.CloseConnection) != 0)
await connection.CloseAsync(ioBehavior).ConfigureAwait(false);
// clear fields (so that MySqlConnection can be GCed if the user doesn't hold a reference to it)
Command = null;
.

@jayrowe
Copy link

jayrowe commented Apr 21, 2024

This breaks pretty reliably against a local MySql instance on my machine:

using MySqlConnector;
using System.Data;

var connectionString = Environment.GetEnvironmentVariable("CONNECTION_STRING");
async Task RunOnce(int instance)
{
    for (var index = 0; index < 1000; index++)
    {
        try
        {
            await using var connection = new MySqlConnection(connectionString);

            await connection.OpenAsync();

            using var command = connection.CreateCommand();

            command.CommandText = $"SELECT * FROM information_schema.columns LIMIT 1;";

            using var reader = await command.ExecuteReaderAsync(
                CommandBehavior.SequentialAccess | CommandBehavior.SingleResult | CommandBehavior.CloseConnection);

            connection.Close();

            await Task.Delay(10);
            await Task.Yield();
        }
        catch (Exception e)
        {
            Console.Error.WriteLine();
            Console.Error.WriteLine($"{instance}, {index}) " + e.ToString());
        }
    }
}

var tasks = new Task[Environment.ProcessorCount * 2];

for (var index = 0; index < tasks.Length; index++)
{
    var local = index;
    tasks[index] = Task.Run(() => RunOnce(local));
}

await Task.WhenAll(tasks);

@bgrainger
Copy link
Member

Thanks @jayrowe; that's a great test case. I was able to simplify the inner loop down to:

using var connection = new MySqlConnection(connectionString);
connection.Open();
using var command = connection.CreateCommand();
command.CommandText = $"SELECT 1;";
using var reader = command.ExecuteReader();
connection.Close();

await Task.Delay(10);

This crashes with 2.3.0 but not 2.2.7. The key part appears to be disposing the MySqlDataReader (via using) after the connection has been closed (which returns it to the pool). This ends up using the same object simultaneously from two different threads (which is the problem).

@ikpil
Copy link

ikpil commented Apr 22, 2024

We are experiencing the exact same issue when upgrading from 2.2.7 to 2.3.x.
Thank you for fixing it.

Once the library is patched, I will also apply the patch and let you know the results.

@bgrainger
Copy link
Member

This is fixed in 2.3.7.

@peppy
Copy link

peppy commented May 7, 2024

Coming from the releases page, we're still getting "Packet received out-of-order" errors (likely in part related to having ProxySQL in front of our database), rolling back to 2.1.6 is an interim solution.

I'm not sure if this is already/still being tracked somewhere and don't immediately have the time to look into it, but thought it may be worth mentioning.

MySqlConnector.MySqlProtocolException
Packet received out-of-order. Expected 1; got 2.

at https://github.com/ppy/osu-queue-score-statistics/blob/33df310f3d34a56d38a3b354d91adf61c2b53ad4/osu.Server.Queues.ScoreStatisticsProcessor/ScoreStatisticsQueueProcessor.cs#L189

@alexr17
Copy link

alexr17 commented Sep 23, 2024

I can also confirm that we hit this after upgrading to 2.3.7, packet out of order + NRE described above.

We do our own complex pooling however so it will take a while to get to the bottom of this. We definitely keep all of our connections open as long as possible, so we aren't intentionally disposing of our connections.

Will update if we can track it down.

@alexr17
Copy link

alexr17 commented Sep 30, 2024

Here is a repro of what we are doing/failing on:

public async Task TestUseReaderAfterDisposal2()
{
    MySqlDataReader firstReader = null;
    MySqlDataReader prevReader = null;
    MySqlConnection connection2 = new MySqlConnection
    {
        // false means pooling disabled
        ConnectionString = GetConnectionString(false),
    };
    
    // Keep the connection open
    await connection2.OpenAsync();
    for (int i = 0; i < 1000; ++i)
    {
        using (var command = connection2.CreateCommand())
        {
            command.CommandText = "SELECT * from mysql.user;";
            command.CommandType = CommandType.Text;

            // Execute command and process result
            using (var secondReader = await command.ExecuteReaderAsync())
            {
                if (firstReader == null)
                {
                    firstReader = secondReader;
                }
                else if (!firstReader.IsClosed || (prevReader?.IsClosed == false))
                {
                    // Throws in 2.3.0+ because the same reader instance is being used for the next command
                    throw new InvalidOperationException("Reader reports not closed after dispose.");
                }

                while (await secondReader.ReadAsync())
                {
                    secondReader.GetValue(0);
                }

                prevReader = secondReader;
            }
        }
    }
}

We are exchanging the reader before disposing it and checking if it is closed afterwards. I believe this breaks the pattern defined here: https://mysqlconnector.net/troubleshooting/connection-reuse/

My opinion is that the recycling of the reader object can lead to potential pitfalls, and I suspect that others may rely on a similar pattern to enable concurrent reads.

@bgrainger
Copy link
Member

bgrainger commented Oct 8, 2024

@alexr17 I think your repro boils down to this; is that correct?

MySqlDataReader usedReader; // will be used to capture the reader
using (var command = new MySqlCommand("select * from mysql.user", connection))
using (var reader = command.ExecuteReader())
{
	while (reader.Read())
	{
	}
	usedReader = reader;
	Console.WriteLine(usedReader.IsClosed); // False
}

Console.WriteLine(usedReader.IsClosed); // True

using (var command = new MySqlCommand("select * from mysql.user", connection))
using (var reader = command.ExecuteReader())
{
	Console.WriteLine(usedReader.IsClosed); // False, expected to stay True
	while (reader.Read())
	{
	}
}

This was noted in #1277 as a side-effect:

This could allow a user to start using a disposed reader without failure (once it's been handed out to a different caller). That currently fails with an ObjectDisposedException, so no existing code would be broken, but people could start writing new, broken code.

It is incorrect to use a MySqlDataReader after it's been disposed (by a using block). (Especially since you have to explicitly capture it and promote it outside of the using scope.) Is this reflective of your real production code?

@alexr17
Copy link

alexr17 commented Oct 14, 2024

@bgrainger Yes this is what we were doing in prod, obviously have changed it now. It's weird that we never hit ObjectDisposedException, only NRE. Although we did upgrade from 2.1.0 straight to 2.3.7

@bgrainger
Copy link
Member

There may have been code paths that didn't trigger ObjectDisposedException; perhaps that was an incorrect assumption on my part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

8 participants