Improve interrupt behavior

[jimhester]

Right now we check for interrupts on the R side, but don't attempt to do anything to signal to the database that an interrupt occurred.

Either calling SQLCancel() on the statement, or SQLFreeStmt() should do it, though we will need to test to make sure.

If that doesn't work then we may have to switch to using async queries, though I would prefer to avoid that complexity if possible.

[detule]

Hey Jim -

Isn't SQLCancel called by nanodbc when some of the resources are destroyed?

If so, I wonder if we can get there by looking into opportunities to release if interrupted.

[jimhester]

yeah, I think that would likely work. Rcpp throws a Rcpp::internal::InterruptedException if R is interrupted from Rcpp::checkUserInterrupt(). So if we put a try / catch block around

odbc/src/odbc_result.cpp

Line 158 in 731b46f

Rcpp::checkUserInterrupt();

, release the statement if interrupted and the re-throw the exception I think that might work.

[hadley]

I don't think the problem is with retrieving the results; it's submitting the query.

[detule]

That makes sense; I think the checkUserInterrupt call that Jim referenced above is also at the point where the query is being submitted for execution, prior to any fetching. He is probably right here, catching the Rcpp exception and making sure we clean up any nanodbc resources may be a good first pass.

Should we try it? Is there a way to test if that helps with the issue. Happy to submit the patch.

[hadley]

@isteves do you happen to have a reprex for a slow query that we could use for testing?

[hadley]

Oh here's a good one from @krlmlr: rstudio/rstudio#8865 (comment)

[detule]

Hey @hadley, @jimhester:

I don't think anything we do here, short of real async, can make the query in that example interruptible, right ? (just making sure that's not the expectation ) At any rate, in terms of improving interrupt handling, with that example I get:

> dbGetQuery(conn, make_query(0:21))
^C^C
> dbGetQuery(conn, "SELECT GETDATE()")
1 2021-02-02 01:18:44
Warning message:
In new_result(connection@ptr, statement, immediate) :
  Cancelling previous query

I think we can make it so that we release the resources when the interrupt is caught (rather than on subsequent query). Does that sound reasonable?

[jimhester]

@detule, that sounds good to me.

@isteves were you using odbc when you had your interruption issues or some other connection package?

[isteves]

@jimhester in the past I used DBI+RPostgreSQL and now I use odbc. I had issues with both though haven't checked the former in a few months now.

[hadley]

@detule just to be clear — what you're saying is that query will continue to run on the database, but control will immediately return to R? How much work would a real async approach be?

[detule]

@hadley - No, I think the goal of wrapping the existing checkUserInterrupt calls (and/or introducing a few more) in try-catch blocks, would be to make sure that we reset the SQL connection in a usable state when control is returned from the I/O call, not immediately. [This is an improvement relative to the current state where no resource cleanup happens when a user interrupt is caught]

I think for control to immediately return to [R] some form of actual async is needed. For that, ODBC has some facilities for asynchronous execution, and these are exposed via nanodbc. But I mean there are so many issues with various drivers when it comes to vanilla execution, let alone async.

Alternatively, one could go the C++ route, i think at this point C++11 is required to build package:odbc. Something like this could work, but of course devil may be in the details, especially when it comes to exception handling in a multi-threaded context!

[hadley]

@detule that sounds like good a thing to do, but it's not the motivation behind this issue (so I don't want it to get lost).

[simonpcouch]

Closed via #796.🥳