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.

Sign up for GitHub

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

Jump to bottom

odbc_result/connection: better reference accounting #731

Merged
merged 4 commits into from
Jan 20, 2024
Merged

odbc_result/connection: better reference accounting #731

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
653219f
odbc_result/connection: better reference accounting
detule Jan 17, 2024
127dbf8
update NEWS
detule Jan 17, 2024
a6f3936
Update NEWS.md
detule Jan 17, 2024
da25d60
Merge branch 'main' into fixup/freetds
detule Jan 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# odbc (development version)

* freetds: Fix regression when executing multiple queries
(@detule, #731)
detule marked this conversation as resolved.
Show resolved Hide resolved

* Oracle: Fix checking for existence when identifier components
contain underscores (@detule, #712).

Expand Down
1 change: 1 addition & 0 deletions src/odbc_connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class odbc_result;

class odbc_connection {
public:
friend odbc_result;
odbc_connection(
std::string connection_string,
std::string timezone = "UTC",
Expand Down
6 changes: 6 additions & 0 deletions src/odbc_result.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,12 @@ odbc_result::~odbc_result() {
try {
c_->set_current_result(nullptr);
} catch (...) {
// SQLCancel may throw an error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - thanks!

// Regardless, as this object is
// getting destroyed, we need to
// make sure the connection is not
// holding onto an invalid reference
c_->current_result_ = nullptr;
};
}
}
Expand Down