add progress handler support to sqlite

[nbaztec]

What
Adds progress handler support to sqlite. The feature is already supported by the sqlite driver. This PR exposes it.

[abonander]

@nbaztec major refactors have landed on main since this PR was authored, do you mind rebasing? The code for the SQLite driver was moved to: https://github.com/launchbadge/sqlx/tree/main/sqlx-sqlite

[nbaztec]

@abonander Done

[abonander]

@nbaztec Github is still saying that there's merge conflicts but won't say what they are. Can you try actually rebasing and not just merging main into your branch?

[nbaztec]

@abonander Done

[abonander]

@nbaztec cargo fmt needs to be run.

[nbaztec]

Done

[abonander]

Sorry it's taken me this long to get to, but unfortunately this PR has several instances of possible undefined behavior that need to be addressed.

Additionally, I think it's worth discussing the actual use-cases for setting a progress handler and if we can design an API that's better tailored to them.

[nbaztec]

The main motivation of the feature would be to enable long-running queries to be interrupted within a reasonable time-frame. As of now, there's no way to abort a SQLite query that is running for a long while.

Thanks for the good review comments.

I'd suggest we store the handler within the LockedSqliteHandle as Option<Box<dyn FnMut() -> bool + Send>>, with an impl Drop that removes the progress handler, since we'd move the progress_handler functionality to LockedSqliteHandle anyhow.

And definitely a good point about catching the panic. Makes sense to use catch_unwind and perhaps treat the panic as an interrupt (optionally print it out).

extern "C" fn progress_callback<F>(callback: *mut c_void) -> c_int
where
    F: FnMut() -> bool,
{
    unsafe {
       let r = catch_unwind(|| {
                let boxed_callback: *mut F = callback.cast::<F>();
                (*boxed_callback)()
       });
       c_int::from(r.unwrap_or_default())
    }
}
            

I saw this pattern being used in a bunch of other sqlite rust implementations like sqlite and rusqlite so reckoned it to be a "safe enough".

[abonander]

I'd suggest we store the handler within the LockedSqliteHandle as Option<Box<dyn FnMut() -> bool + Send>>, with an impl Drop that removes the progress handler, since we'd move the progress_handler functionality to LockedSqliteHandle anyhow.

That's probably still UB as I addressed in my follow-up comment. Box implies that the pointer is not aliased, which it very much is while being stored inside SQLite. If we never ever touched the Box while it's aliased, then maybe, but that's a potentially big problem if it goes awry.

I would store it as Option<NotNull<dyn FnMut() -> bool + Send>> as you can have raw pointers to trait objects, and then yeah an impl Drop to remove the progress handler and destroy the value on-drop by just doing let _ = unsafe { Box::from_raw(pointer) };.

[abonander]

It doesn't make sense to store it in the LockedSqliteHandle though as that just wraps a MutexGuard to ConnectionState. Just put it in ConnectionState please.

[nbaztec]

@abonander I tried to apply the changes but stumbled onto the problem that the dyn FnMut needs to be Sized. I can't figure out a way to do it without introducing a Box or a generic param (which touches quite a bit of code). Any ideas?

[abonander]

@nbaztec Option<NonNull<dyn FnMut(...) -> ...>> should be fine, did you forget the NonNull part?

[nbaztec]

Had some other problem - PEBKAC. Managed to remedy it. Please review the new code, happy to make any further improvements.

[abonander]

Please pay close attention to what you're doing inside unsafe {} blocks, and ensure to address all review comments this time.