Copilot commented on code in PR #4134:
URL: https://github.com/apache/arrow-adbc/pull/4134#discussion_r2984748500
##########
csharp/src/Client/AdbcDataReader.cs:
##########
@@ -324,6 +315,9 @@ protected override void Dispose(bool disposing)
{
this.recordBatch?.Dispose();
this.recordBatch = null;
+ this.adbcQueryResult.Stream?.Dispose();
+ this.adbcQueryResult.Stream = null;
+ this.isClosed = true;
}
Review Comment:
`Dispose(bool)` does not honor `closeConnection`. If callers use
`using`/`Dispose()` on a reader created with `CommandBehavior.CloseConnection`,
the connection will remain open because only `Close()` closes it. Consider
moving the `closeConnection` logic into `Dispose(bool)` (and have `Close()`
simply call `Dispose()`), so `Close()` and `Dispose()` are equivalent as
ADO.NET expects.
```suggestion
// Ensure that any logic implemented in Close(), including
honoring
// CommandBehavior.CloseConnection, also runs when Dispose()
is called.
this.Close();
}
base.Dispose(disposing);
```
##########
csharp/src/Client/AdbcDataReader.cs:
##########
@@ -138,9 +138,7 @@ public override void Close()
{
this.adbcCommand?.Connection?.Close();
}
- this.adbcQueryResult.Stream?.Dispose();
- this.adbcQueryResult.Stream = null;
- this.isClosed = true;
+ this.Dispose(disposing: true);
}
Review Comment:
There are existing client/driver tests for reader behavior, but nothing
appears to cover the `CommandBehavior.CloseConnection` contract. Since this
change refactors `Close()`/`Dispose()` behavior and stream disposal, please add
a test asserting that disposing the reader closes the connection when
`CloseConnection` is specified (and that disposing without it leaves the
connection open).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
