Skip to content

[Lock] Release PostgreSqlStore connection lock on failure #44723

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.

Sign up for GitHub

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

Jump to bottom
Merged
nicolas-grekas merged 1 commit into from
Dec 28, 2021
Merged

[Lock] Release PostgreSqlStore connection lock on failure #44723

nicolas-grekas merged 1 commit into from
Dec 28, 2021

Conversation

@simon-watiau
Copy link
Contributor

@simon-watiau simon-watiau commented Dec 20, 2021 •
edited
Loading

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

When using PostgreSQL advisory locks using the PostgreSqlStore
A first lock is acquired in memory for same connection concurrencies, this InMemoryStore does not rely on TTL.

When the advisory lock fails to be acquired, this first lock is not released.

For long running processes, this cause the lock to not be acquirable again because the InMemoryStore will never release its lock.

@carsonbot
Copy link

carsonbot commented Dec 20, 2021

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@simon-watiau simon-watiau marked this pull request as ready for review December 20, 2021 11:05
@carsonbot carsonbot added this to the 5.4 milestone Dec 20, 2021
@carsonbot
Copy link

carsonbot commented Dec 20, 2021

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@fancyweb fancyweb added the Lock label Dec 20, 2021
@carsonbot carsonbot changed the title fix(Lock): Release connection lock on failure [Lock] fix(Lock): Release connection lock on failure Dec 20, 2021
@nicolas-grekas nicolas-grekas changed the title [Lock] fix(Lock): Release connection lock on failure [Lock] Release connection lock on failure Dec 26, 2021
nicolas-grekas
nicolas-grekas approved these changes Dec 26, 2021
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(for 5.3)

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 5.3 Dec 26, 2021
jderusse
jderusse reviewed Dec 26, 2021
View reviewed changes
@simon-watiau simon-watiau changed the base branch from 5.4 to 5.3 December 27, 2021 09:23
@simon-watiau
Copy link
Contributor Author

simon-watiau commented Dec 27, 2021

@nicolas-grekas I rebased it on the 5.3 branch

@GromNaN
Copy link
Member

GromNaN commented Dec 28, 2021

Thanks for this fix @simon-watiau. I'm wondering if we can add a test in AbstractStoreTest for that use-case.

The DoctrineDbalPostgreSqlStore has the same behaviour and should be updated too in a PR against 5.4.

@simon-watiau simon-watiau mentioned this pull request Dec 28, 2021
Merged
@simon-watiau simon-watiau changed the title [Lock] Release connection lock on failure [Lock] Release PostgreSqlStore connection lock on failure Dec 28, 2021
@simon-watiau
Copy link
Contributor Author

simon-watiau commented Dec 28, 2021

@GromNaN I added a test, however I kept it in PostgreSqlStoreTest as this issue concerns store instances that share a common backend (which does not apply to InMemoryStore for instance)

I also opened another PR on 5.4 with the same fix #44828 for DoctrineDbalPostgreSqlStore

nicolas-grekas
nicolas-grekas reviewed Dec 28, 2021
View reviewed changes
src/Symfony/Component/Lock/Tests/Store/PostgreSqlStoreTest.php Outdated
}

/**
* @requires extension pdo_sqlite
Copy link
Member

@nicolas-grekas nicolas-grekas Dec 28, 2021

Choose a reason for hiding this comment

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

not needed I suppose

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 28, 2021

Thank you @simon-watiau.

@nicolas-grekas nicolas-grekas merged commit 83735c4 into symfony:5.3 Dec 28, 2021
@simon-watiau simon-watiau deleted the fix/PostgreSqlStore-not-releasing-session-locks branch December 28, 2021 16:29
nicolas-grekas added a commit that referenced this pull request Dec 29, 2021
@nicolas-grekas
bug #44828 [Lock] Release DoctrineDbalPostgreSqlStore connection lock…
5c5ac42 
… on failure (simon-watiau)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Lock] Release DoctrineDbalPostgreSqlStore connection lock on failure

| Q             | A
| ------------- | ---
| Branch?       | 5.4<!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | <!-- required for new features -->

When using PostgreSQL advisory locks using the `DoctrineDbalPostgreSqlStore`
A first lock is acquired in memory for same connection concurrencies, this `InMemoryStore` does not rely on TTL.

When the advisory lock fails to be acquired, this first lock is not released.

For long running processes, this cause the lock to not be acquirable again because the `InMemoryStore` will never release its lock.

related to : #44723 (comment)

Commits
-------

fab5991 [Lock] Release DoctrineDbalPostgreSqlStore connection lock on failure
This was referenced Dec 29, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@jderusse jderusse Awaiting requested review from jderusse jderusse is a code owner

Assignees

No one assigned

Labels

Bug Lock Status: Reviewed

Projects

None yet

Milestone

5.3

Development

Successfully merging this pull request may close these issues.

6 participants

@simon-watiau @carsonbot @GromNaN @nicolas-grekas @jderusse @fancyweb