Skip to content

fix(postgres): make advisory lock cancel safe#4199

Open
joeydewaal wants to merge 2 commits intolaunchbadge:mainfrom
joeydewaal:pg/advisory-lock
Open

fix(postgres): make advisory lock cancel safe#4199
joeydewaal wants to merge 2 commits intolaunchbadge:mainfrom
joeydewaal:pg/advisory-lock

Conversation

@joeydewaal
Copy link
Contributor

This pr makes the PgAdvisoryLock::acquire cancel safe by wrapping the connection in a PgAdvisoryLock before actually acquiring the lock. If the acquire future is dropped the drop impl will release the lock.

Does your PR solve an issue?

fixes #4198

Is this a breaking change?

Nope

pub async fn acquire<C: AsMut<PgConnection>>(&self, conn: C) -> Result<PgAdvisoryLockGuard<C>> {
// We're wrapping the connection in a `PgAdvisoryLockGuard` early here on purpose. If this
// future is dropped, the lock will be released in the drop impl.
let mut guard = PgAdvisoryLockGuard::new(self.clone(), conn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this also cause us to release the lock on drop before it's even been acquired? Is that OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nvm, I assume anything done before the first await happens atomically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, is it possible that the first await yields before actually managing to send the lock query? Like maybe if the TCP send buffer is full?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the future can be dropped before it's actually sent out. But then it's in the write buffer, which is flushed the next time before the connection is used.

I guess the worst case would be when the query isn't prepared. Then the drop is going to queue an unnecessary release. Thanks for calling this out.

@joeydewaal
Copy link
Contributor Author

joeydewaal commented Mar 24, 2026

Note: I've decided to not update the try_acquire implementation because releasing the lock depends on the result of the query. So that method is still not cancel safe unfortunately.

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.

PgAdvisoryLock::acquire is not cancel safe (undocumented footgun)

2 participants