Skip to content

[Lock] Reset Key lifetime time before we acquire it #38553

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

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Oct 13, 2020

Q A
Branch? 5.1 (maybe lower, I'll check)
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #38541
License MIT
Doc PR n/a

Im out on somewhat deep water now. I am pretty sure we should reset the Key lifetime every time we acquire it. Without it it will me tricky to re-use a lock. (As pointed out by #38541)

@jderusse can you confirm.

Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

We probably have the same bug in 5.2 with acquireRead

@fabpot
Copy link
Member

fabpot commented Oct 14, 2020

Which branch should we target? 4.4?

@jderusse
Copy link
Member

This should target 4.4.

@fabpot
Copy link
Member

fabpot commented Oct 14, 2020

@Nyholm Can you rebase?

@Nyholm Nyholm force-pushed the lock-reset-lifetime branch from 86f6f53 to f87c11e Compare October 14, 2020 06:45
@Nyholm Nyholm changed the base branch from 5.x to 4.4 October 14, 2020 06:45
@Nyholm Nyholm force-pushed the lock-reset-lifetime branch 3 times, most recently from ab6fc4d to 54b7e98 Compare October 14, 2020 06:49
@Nyholm Nyholm force-pushed the lock-reset-lifetime branch from e9fc16c to 55ad702 Compare October 14, 2020 07:15
@Nyholm
Copy link
Member Author

Nyholm commented Oct 14, 2020

Yes, rebased and squashed.

fabpot added a commit that referenced this pull request Oct 14, 2020
This PR was merged into the 5.x branch.

Discussion
----------

[Lock] Reset lifetime on acquireRead()

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        | n/a

Same as #38553 but this is for `acquireRead()` instead of `acquire()`.

`acquireRead()` is new in 5.2.

Commits
-------

de412bf Reset lifetime on acquireRead()
@fabpot
Copy link
Member

fabpot commented Oct 14, 2020

Thank you @Nyholm.

@fabpot fabpot merged commit 674382b into symfony:4.4 Oct 14, 2020
@xabbuh xabbuh added this to the 4.4 milestone Oct 14, 2020
@fabpot fabpot mentioned this pull request Oct 14, 2020
Nyholm added a commit that referenced this pull request Oct 15, 2020
…jderusse)

This PR was merged into the 5.x branch.

Discussion
----------

[Semaphore] Reset Key lifetime time before we acquire it

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | /
| License       | MIT
| Doc PR        | /

This is the same issue fixed by #38553 but for Semaphore component.

Commits
-------

e7ffd5d Reset Key lifetime time in semaphore
This was referenced Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lock with RedisStore: LockAcquiringException on second acquisition of lock within the same process (after TTL)
5 participants