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

AdvisoryLockGuard without lifetime #3429

Open
bonsairobo opened this issue Aug 12, 2024 · 4 comments · May be fixed by #3495
Open

AdvisoryLockGuard without lifetime #3429

bonsairobo opened this issue Aug 12, 2024 · 4 comments · May be fixed by #3495
Labels
db:postgres Related to PostgreSQL enhancement New feature or request

Comments

@bonsairobo
Copy link

bonsairobo commented Aug 12, 2024

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.

@bonsairobo bonsairobo added the enhancement New feature or request label Aug 12, 2024
@abonander
Copy link
Collaborator

abonander commented Aug 12, 2024

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
Copy link
Author

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
Copy link
Collaborator

Yeah, I'd appreciate it!

@bonsairobo
Copy link
Author

bonsairobo commented Aug 19, 2024

@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?

@bonsairobo bonsairobo changed the title Reference-counting AdvisoryLockGuard AdvisoryLockGuard without lifetime Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
db:postgres Related to PostgreSQL enhancement New feature or request
Projects
None yet
2 participants