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

A way to avoid panic on drop behavior? #1292

Closed
mhammond opened this issue Feb 23, 2023 · 3 comments · Fixed by #1293
Closed

A way to avoid panic on drop behavior? #1292

mhammond opened this issue Feb 23, 2023 · 3 comments · Fixed by #1293

Comments

@mhammond
Copy link
Contributor

mhammond commented Feb 23, 2023

Hi,
We are using rusqlite (indirectly) behind an Arc<> in a context where panic=abort (desktop Firefox). As a result, because close() consumes self, we can't absolutely guarantee that at shutdown time we can Arc::try_unwrap() to get ownership of the connection to call close() on and mem::forget it on failure. This means there's a chance that as the app shuts down the app will abort as it is dropped. We'd much rather leak the connection than abort.

I assume the panic is desirable in some use-cases, otherwise I'd suggest that on drop rusqlite either ignores the error or uses sqlite_close_v2. I'm not sure in which contexts it would actually be desirable though - if consumers really want to notice the error they still have the explicit close available.

Alternatively, maybe a new boolean could be introduced 👋 somewhere 👋 to control this behavior?

Either way, I'd be happy to put a PR up once I know what approach, if any, would be likely to be accepted.

@gwenn
Copy link
Collaborator

gwenn commented Feb 23, 2023

Just for reference, rust-postgres doesn't panic.

Rusqlite doesn't provide any binding (yet) to sqlite3_trace_v2 but users can use SQLITE_TRACE_CLOSE to trap close event.

sqlite3_close_v2:

The sqlite3_close_v2() interface is intended for use with host languages that are garbage collected, and where the order in which destructors are called is arbitrary.

Rust is not gced and, afaik, the order of destructors is deterministic.

@thomcc
Copy link
Member

thomcc commented Feb 23, 2023

I think it's reasonable for us to not panic on drop ever. In general that behavior is pretty discouraged now in the standard library, which probably we should follow.

@mhammond
Copy link
Contributor Author

Yeah, I did read that sqlite_close_v2() was intended for GC languages, but it also says it 'makes arrangements to automatically deallocate the database connection after all prepared statements are finalized, all BLOB handles are closed, ...' - it's not clear that would actually happen in this scenario, but it thought it sounds like it can't hurt :)

But thanks, I'll put up a trivial PR to just not panic on drop.

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 a pull request may close this issue.

3 participants