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

Do not close the inner reader when disposing wrapped data readers #2100

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Jul 3, 2024

When disposing wrapped readers, only call Dispose() on the inner reader and let it catch exceptions if appropriate.

Note: this actually happened with Microsoft.Data.SqlClient.

Microsoft.Data.SqlClient.SqlException (0x80131904): Execution Timeout Expired.  The timeout period elapsed prior to completion of the operation or the server is not responding.
 ---> System.ComponentModel.Win32Exception (258): The wait operation timed out.
   at void Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, bool breakConnection, Action<Action> wrapCloseInAction)
   at void Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, bool callerHasConnectionLock, bool asyncClose)
   at bool Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, out bool dataReady)
   at bool Microsoft.Data.SqlClient.SqlDataReader.TryCloseInternal(bool closeReader)
   at void Microsoft.Data.SqlClient.SqlDataReader.Close()
   at void Dapper.DbWrappedReader.Dispose(bool disposing) in /_/Dapper/WrappedReader.cs:line 149
   at ValueTask System.Data.Common.DbDataReader.DisposeAsync()

When disposing wrapped readers, only call Dispose() on the inner reader and let it catch exceptions if appropriate.

Note: this actually happened with `Microsoft.Data.SqlClient`.

```
Microsoft.Data.SqlClient.SqlException (0x80131904): Execution Timeout Expired.  The timeout period elapsed prior to completion of the operation or the server is not responding.
 ---> System.ComponentModel.Win32Exception (258): The wait operation timed out.
   at void Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, bool breakConnection, Action<Action> wrapCloseInAction)
   at void Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, bool callerHasConnectionLock, bool asyncClose)
   at bool Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, out bool dataReady)
   at bool Microsoft.Data.SqlClient.SqlDataReader.TryCloseInternal(bool closeReader)
   at void Microsoft.Data.SqlClient.SqlDataReader.Close()
   at void Dapper.DbWrappedReader.Dispose(bool disposing) in /_/Dapper/WrappedReader.cs:line 149
   at ValueTask System.Data.Common.DbDataReader.DisposeAsync()
```
@mgravell
Copy link
Member

mgravell commented Jul 3, 2024

Seems fair; we can let the inner reader worry about what Dispose() means, since we're propagating Dispose(), not Close() + Dispose().

@mgravell
Copy link
Member

mgravell commented Jul 3, 2024

overriding appveyor, although the pgsql problem might prevent "main" deploy - that's not your problem, though; unrelated to this PR

@mgravell mgravell merged commit 9ed3525 into DapperLib:main Jul 3, 2024
1 of 2 checks passed
@0xced 0xced deleted the DisposeNoThrow branch July 3, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants