Efcore: BUG in RelationalDataReader::Dispose

[dmitry-lipetsk]

Hello,

EF Core version: dev

RelationalDataReader::Dispose calls _reader.RecordsAffected after call of _reader.Dispose();

        public virtual void Dispose()
        {
            if (!_disposed)
            {
                _reader.Dispose();
                _command.Parameters.Clear();
                _command.Dispose();
                _connection.Close();

                _logger.DataReaderDisposing(
                    _connection,
                    _command,
                    _reader,
                    _commandId,
                    _reader.RecordsAffected,
                    _readCount,
                    _startTime,
                    _stopwatch.Elapsed);

                _disposed = true;
            }
        }

My DataReader does not allows work with him after Dispose and generates ObjectDisposedException exception.

I thing, _logger.DataReaderDisposing must be called before disposing of child objects, not after.

[smitpatel]

We moved the logging after all the dispose so that failure in logging does not cause data leaks.

[dmitry-lipetsk]

You may read and save _reader.RecordsAffected before call of Dispose.

But in any case - this code incorrect.

I offer move _logger.DataReaderDisposing to head and write (hack) code like:

try
{ 
 _logger.DataReaderDisposing(...);
}
catch
{
 //ignore any errors
}

_reader.Dispose();
_command.Parameters.Clear();
_command.Dispose();
_connection.Close();

or

try
{ 
 _logger.DataReaderDisposing(...);
}
finally
{
 _reader.Dispose();
 _command.Parameters.Clear();
 _command.Dispose();
 _connection.Close();
}

[dmitry-lipetsk]

If you remove a readonly from _reader, _command, _connection, you may write more accurrate code:

        public virtual void Dispose()
        {
            if (!_disposed)
            {
                var local_reader
                 =Interlocked.Exchange(ref _reader,null);

                var local_command
                 =Interlocked.Exchange(ref _command,null);

                var local_connection
                 =Interlocked.Exchange(ref _connection,null);

                _disposed = true;

                try
                {
                 _logger.DataReaderDisposing(
                     local_connection,
                     local_command,
                     local_reader,
                     _commandId,
                     local_reader.RecordsAffected,
                     _readCount,
                     _startTime,
                     _stopwatch.Elapsed);
                }
                finally
                {
                 local_reader.Dispose();
                 local_command.Parameters.Clear();
                 local_command.Dispose();
                 local_connection.Close();
                }
            }
        }

[ajcvickers]

Triage: we will refactor the code for safety. Note that this does not need to be done in a thread-safe manner.

[roji]

Note that according to the docs:

The RecordsAffected property is not set until all rows are read and you close the DataReader.

So it's probably best to close the reader, access RowsAffected, and then dispose it.