odbc_result/connection: better reference accounting

[detule]

Fixes #716

[hadley]

Are you sure this code shouldn't belong in odbc_connection::cancel_current_result()?

Re-looking at this code also made me realise that the warning() is potentially dangerous because if options(warn = 2) is set, then we'll never get to the cancel(). We should probably add a test for that case too, using something like withr::local_options(warn = 2).

[detule]

Hey Hadley - thanks for taking a look.

I think this is a good spot for it. We could try/catch closer to the place that throws currently. But I think we still need to update the exception handling here in case another piece of code throws ( now, or in the future, if we were to expand set_current_result / etc ).

[hadley]

It feels weird to me because the job of c_->set_current_result(nullptr) is to reset the current result, and now its purpose is leaking out side of its scope. i.e. I think it's a code smell that you have to use friend here in order to reach inside the implementation detail of another function.

[detule]

Hi Hadley - thank you for continuing to think about this.

I think using friend here is OK: the odbc_connection/odbc_result relationship is more than a bit incestuous to begin with, with both holding a (private) reference to each other. Looks like we needed to forward declare the objects just to get the code to compile.

In terms of leaking purpose: set_current_result has two jobs - cancel the current statement with some caseology sprinkled in ( throws ) and second, reset the pointer. As we can't throw in the destructor, we could try and separate the calls - for example, wrap that second role in a oneliner public void reset_result_ptr( odbc_result* r ) noexcept method. This would make it also so that we do not need to make the classes friends. But I think I would probably still call that method in the destructor over adding exception handling in set_current_result. The destructor might look something like this:

try {
        c_->cancel_current_result(true);
} catch (...) {
};
c_->reset_result_ptr(nullptr);

Anyway - not really tied to anything here. Happy to go in a different direction.

[hadley]

I was thinking about this being a destructor last night — I think that means we can't call Rcpp::warning() safely at all here, because it might long jump, and while I can't remember exactly what happens when you long jump out of C++ destructor I know it's not good.

How about we merge this PR to fix the pressing problem and then file another issue to rethink the design of this system to be safer?

[detule]

Sounds good - thanks!

[simonpcouch]

I only have as much intuition on the C++ changes as ChatGPT could give me, but I can confirm that I don't see the segfault from the linked issue with these changes. :)