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.

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

Adds a method get the underlying sqlite3 pointer #438

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Adds a method get the underlying sqlite3 pointer #438

merged 2 commits into from
Jun 24, 2020

Conversation

agentsim
Copy link
Contributor

Per #430, adds a method as_raw_handle() to get the underlying sqlite3* from a SqliteConnection.

This PR is cleaner (a single commit). The previous PR also failed CI due to a (hopefully) random fault in docker.

@mehcode
Copy link
Member

mehcode commented Jun 23, 2020

failed CI due to a (hopefully) random fault in docker.

GitHub Actions is randomly failing to run docker-compose run .... It looks like a potential race condition with it starting the docker daemon? Not sure.


The PR looks good to me except.. I think we should require &mut self as you're returning a *mut sqlite3 there. Thoughts?

@agentsim
Copy link
Contributor Author

I left it as &self since as_ptr() on ConnectionHandle takes &self and I assumed there was a reason for that.

@abonander
Copy link
Collaborator

I believe it's an antipattern to have a method take &self and return *mut Foo. It looks like SqliteConnectionHandle::as_ptr() can be changed to take &mut self and all usages of it should still compile.

It's especially important to take &mut self for the public API, I think, as we probably want to enforce that the connection isn't currently being used.

@@ -33,6 +34,13 @@ pub struct SqliteConnection {
scratch_row_column_names: Arc<HashMap<UStr, usize>>,
}

impl SqliteConnection {
/// Returns the underlying sqlite3* connection handle
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we need to list some do-s and don't-s for this pointer but I'm not sure what those should be.

Copy link
Contributor Author

@agentsim agentsim Jun 23, 2020

Choose a reason for hiding this comment

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

The obvious two that come to mind are not to close the connection or free the pointer. I don't think the Rust docs in the standard library for similar low-level things spell out the do-s and don't-s.

@mehcode mehcode merged commit 72c4e04 into launchbadge:master Jun 24, 2020
@mehcode
Copy link
Member

mehcode commented Jun 24, 2020

Thanks for the addition. We can always improve docs as we move along. Maybe just a general warning like "preserve the pointer and don't close the connection or bad things will happen".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants