Fix leak of PreparedStatement on query failure #663
Conversation
|
Reviewing is easiest if you hide whitespace changes |
| * otherwise we do not clean up the prepared statement and various | ||
| * other DuckDB objects. | ||
| */ | ||
| CleanupDuckdbScanState(duckdb_scan_state); |
There was a problem hiding this comment.
The usual way to do that is to use RAII. We could add this trivial struct above in the file:
struct ScopedDuckDBScanState {
ScopedDuckDBScanState(_state) : state(_state);
~ScopedDuckDBScanState() {
CleanupDuckdbScanState(state);
}
private:
DuckdbScanState *state;
}And then at the beginning of this function
DuckdbScanState *duckdb_scan_state = (DuckdbScanState *)node;
ScopedDuckDBScanState(duckdb_scan_state);There was a problem hiding this comment.
But that would also clean it up on success right? We don't want to do that, because this function is called many times. If the function completes successfully CleanupDuckdbScanState will already be called in Duckdb_EndCustomScan_Cpp.
There was a problem hiding this comment.
Updated the comment to reflect this, because indeed it's not immediately obvious.
There was a problem hiding this comment.
Ah yes, I should have read more carefully.
We could make the ScopedDuckDBScanState to skip cleanup on success but that would be clunky.
I personally would have:
- renamed
Duckdb_ExecCustomScan_CpptoDuckdb_ExecCustomScan_Cpp_Unsafe - implemented
Duckdb_ExecCustomScan_Cppwith only the try/catch+cleanup
IMHO this makes the whole cleanup logic more apparent (and probably lessen the need for the comment). But happy with that version too
Fixes #662