`AdvisoryLockGuard` without lifetime

[bonsairobo]

Is your feature request related to a problem? Please describe.

I have an issue trying to use AdvisoryLockGuard. The lifetime prevents me from writing code that acquires the lock and then passes the guard into a 'static closure. This is relevant because I want to be able to write an axum request handler that upgrades to a websocket, but I want any errors that occur while acquiring the lock to be sent in a Response instead of a websocket close message.

A rought sketch of how I want the code to look:

async fn my_handler(
    ws: WebSocketUpgrade,
    State(state): State<MyState>,
) -> Result<Response, ApiError> {
    let db = state.get_db();
    let lock = PgAdvisoryLock::new(...);
    let Either::Left(mut guard) = model_lock.try_acquire(&mut db).await? else {
        return Err(ApiError::Foo(...));
    };

    let response = ws.on_upgrade(move |socket| async move {
        // Have the guard in here while doing stuff with the socket...
    });

    Ok(response)
}

Describe the solution you'd like

I think having an API similar to this would work, though I'm not sure how feasible it would be to implement that for sqlx.

Describe alternatives you've considered

AFAICT my only (safe) option is to move the PgAdvisoryLock into the task scope that handles the socket. This means I must return any errors within that scope along the socket instead of in an HTTP response.

[abonander]

If the key string you pass to PgAdvisoryLock is constant, you could put it in a smol::lock::OnceCell or a std::sync::LazyLock inside a static which would give you &'static references to it.

We could probably promote with_key() to a const fn so that it can be constructed directly in a static.

Otherwise, I'm not against .acquire_owned()/.try_acquire_owned() methods taking Arc<Self>. There's no real reason it doesn't exist already besides "I didn't think of it".

Couple bikeshedding things:

• I'd prefer the _owned suffix to match Tokio: https://docs.rs/tokio/latest/tokio/sync/struct.RwLock.html#method.write_owned
• It's a judgement call whether to take Arc<Self> or &Arc<Self>. The latter would increment the reference count lazily for less thrashing on highly contended locks, but the former would allow even the acquire() call itself to be moved into a task if desired.
  • I'm leaning toward Arc<Self> since it's strictly more flexible at the minor cost of requiring an explicit .clone(). Thrashing the refcount doesn't really matter since it's going to be dwarfed by the network overhead anyway. It also matches Tokio's API.

[bonsairobo]

Unfortunately my keys are not constant. But I appreciate the idea.

I agree with your design thoughts.

I might have time to try implementing this feature if you're open to that.

[abonander]

Yeah, I'd appreciate it!

[bonsairobo]

@abonander I'm starting to work on this, and I've had a few realizations.

First, we probably don't need reference counting for this. I think I incorrectly pattern matched when proposing the RwLockReadGuardArc. Postgres advisory locks are more like mutexes, so we don't really need the ability to clone read guards. (EDIT: I realized that postgres advisory locks actually do support shared locking. Seems like we might want to support that at some point. However, because locks are managed by the session, it's still not really necessary to clone read guards; this would require cloning the connection as well, which is never really desirable.)

But maybe there is some desire to use Arc<AdvisoryLock> instead of just AdvisoryLock; at least this is what tests are doing. I have not personally needed to do this. Is this an optimization to avoid deeply cloning the AdvisoryLock?

If we really did want to have a cloneable guard, I think it would also need a mutex around the connection, since most users would need a AsMut<PgConnection> to keep making queries with the lock guard. This seems different enough from the existing API that I don't think it's worth going down that route.

So for now I've opted to support Arc<AdvisoryLock>, but there is no reference counting of the guard itself. We could potentially make the lock field generic if that's desirable.

pub struct PgAdvisoryLockGuardOwned<C: AsMut<PgConnection>> {
    lock: Arc<PgAdvisoryLock>,
    conn: Option<C>,
}

I'm also wondering how we want to handle documentation. It seems like any new documentation would mostly duplicate existing documentation. Same with the tests.

In fact, I wonder if we even want to keep the old PgAdvisoryLockGuard. Does this new one not satisfy all use cases of the old one?

Here's some progress in a draft PR: #3442

Any thoughts?