Skip to content

[8.x] Add extend option to Atomic Locks #32524

Closed
georgeboot wants to merge 3 commits intolaravel:masterfrom
georgeboot:atomic-locks-extend
Closed

[8.x] Add extend option to Atomic Locks #32524
georgeboot wants to merge 3 commits intolaravel:masterfrom
georgeboot:atomic-locks-extend

Conversation

@georgeboot
Copy link
Copy Markdown
Contributor

@georgeboot georgeboot commented Apr 24, 2020

This PR adds support for extending existing atomic locks.

Adapters to implement:

  • Redis
  • Memcached
  • DynamoDB

Use case

Imagine a support system, where only one agent at a time, can work on a case.

When implementing this with atomic locks, you would want to lock a case for 30 seconds, and have the page poll every 15 seconds. That poll request should be able to extend the existing lock.

When the agent leaves the page, the case will automatically be unlocked as soon as the lock expires.

Help wanted

I'm not experiences with DynamoDB, so I can't figure out how to easily extend the TTL of a record. Perhaps someone else wishes to share their wisdom?

@georgeboot georgeboot changed the title [7.x] Adding extend option to Atomic Locks [7.x] [Help wanted] Adding extend option to Atomic Locks Apr 24, 2020
@GrahamCampbell GrahamCampbell changed the title [7.x] [Help wanted] Adding extend option to Atomic Locks [7.x] Adding extend option to Atomic Locks Apr 24, 2020
@GrahamCampbell GrahamCampbell changed the title [7.x] Adding extend option to Atomic Locks [7.x] Add extend option to Atomic Locks Apr 24, 2020
@georgeboot georgeboot changed the base branch from 7.x to master April 24, 2020 10:33
@georgeboot georgeboot changed the title [7.x] Add extend option to Atomic Locks [8.x] Add extend option to Atomic Locks Apr 24, 2020
*/
public function extend($seconds)
{
return (bool)$this->redis->expire($this->name);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does this extend anything? The seconds are literally never used.

public function release()
{
return (bool) $this->redis->eval(LuaScripts::releaseLock(), 1, $this->name, $this->owner);
return (bool)$this->redis->eval(LuaScripts::releaseLock(), 1, $this->name, $this->owner);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why?

@taylorotwell
Copy link
Copy Markdown
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

@georgeboot
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback. Will re-submit when it’s more complete.

@taylorotwell What is the best way for me to get someone collaborating on the DynamoDB part?

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.

3 participants