-
-
Notifications
You must be signed in to change notification settings - Fork 27
lock add heartbeat param #1030
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
lock add heartbeat param #1030
Changes from all commits
7ca1966
fa1d198
0640e20
81a651f
3d82a41
cb65045
022739c
6d03a8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,10 +11,17 @@ | |
|
|
||
| namespace FriendsOfHyperf\Lock\Driver; | ||
|
|
||
| use FriendsOfHyperf\Lock\Exception\HeartbeatException; | ||
| use FriendsOfHyperf\Lock\Exception\LockTimeoutException; | ||
| use Hyperf\Context\ApplicationContext; | ||
| use Hyperf\Contract\StdoutLoggerInterface; | ||
| use Hyperf\Coordinator\Constants; | ||
| use Hyperf\Coordinator\CoordinatorManager; | ||
| use Hyperf\Coroutine\Coroutine; | ||
| use Hyperf\Stringable\Str; | ||
| use Hyperf\Support\Traits\InteractsWithTime; | ||
| use Override; | ||
| use Throwable; | ||
|
|
||
| use function Hyperf\Support\now; | ||
|
|
||
|
|
@@ -32,12 +39,18 @@ abstract class AbstractLock implements LockInterface | |
| */ | ||
| protected int $sleepMilliseconds = 250; | ||
|
|
||
| protected int $heartbeat = 0; | ||
|
|
||
| /** | ||
| * Create a new lock instance. | ||
| */ | ||
| public function __construct(protected string $name, protected int $seconds, ?string $owner = null) | ||
| public function __construct(protected string $name, protected int $seconds, ?string $owner = null, int $heartbeat = 0) | ||
| { | ||
| $this->owner = $owner ?? Str::random(); | ||
| $this->heartbeat = $heartbeat; | ||
| if ($seconds > 0 && $heartbeat > 0 && $seconds <= $heartbeat) { | ||
| throw new HeartbeatException('Heartbeat must be less than lock seconds.'); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -131,6 +144,8 @@ public function isOwnedBy($owner): bool | |
| return $this->getCurrentOwner() === $owner; | ||
| } | ||
|
|
||
| abstract protected function delayExpiration(): bool; | ||
|
|
||
| /** | ||
| * Returns the owner value written into the driver for this lock. | ||
| */ | ||
|
|
@@ -143,4 +158,35 @@ protected function isOwnedByCurrentProcess(): bool | |
| { | ||
| return $this->isOwnedBy($this->owner); | ||
| } | ||
|
|
||
| protected function heartbeat(): bool | ||
| { | ||
| if ($this->heartbeat <= 0 || $this->seconds <= 0 || ! Coroutine::inCoroutine()) { | ||
| return true; | ||
| } | ||
| Coroutine::create(function () { | ||
| while (true) { | ||
| if (CoordinatorManager::until(Constants::WORKER_EXIT)->yield($this->heartbeat)) { | ||
| break; | ||
| } | ||
| if ($this->heartbeat == 0) { | ||
| return; | ||
| } | ||
| if (! $this->isOwnedByCurrentProcess()) { | ||
| return; | ||
| } | ||
| try { | ||
| $this->delayExpiration(); | ||
| } catch (Throwable $throwable) { | ||
| ApplicationContext::getContainer()->get(StdoutLoggerInterface::class)?->warning($throwable); | ||
| } | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
Comment on lines
+162
to
+186
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 关键缺陷:心跳方法未被调用,功能无法生效
但存在关键问题:此方法在整个抽象类中从未被调用。 查看 建议在以下位置之一调用
示例修改(方案 1): public function get(?callable $callback = null)
{
$result = $this->acquire();
+ if ($result) {
+ $this->heartbeat();
+ }
if ($result && is_callable($callback)) {示例修改(方案 2): while (! $this->acquire()) {
$now = ((int) now()->format('Uu')) / 1000;
if (($now + $this->sleepMilliseconds - $milliseconds) >= $starting) {
throw new LockTimeoutException();
}
usleep($this->sleepMilliseconds * 1000);
}
+ $this->heartbeat();
if (is_callable($callback)) {
🤖 Prompt for AI Agents |
||
|
|
||
| protected function stopHeartbeat(): void | ||
| { | ||
| $this->heartbeat = 0; | ||
| } | ||
|
Comment on lines
+188
to
+191
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 缺少 stopHeartbeat() 调用,心跳协程可能无法及时停止
根据之前的评审意见和最佳实践,应在释放锁时停止心跳协程,以防止资源泄漏。 建议在 if ($result && is_callable($callback)) {
try {
return $callback();
} finally {
+ $this->stopHeartbeat();
$this->release();
}
}同样的修改也应应用于 此外,
🤖 Prompt for AI Agents |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,9 +26,9 @@ class CacheLock extends AbstractLock | |
| /** | ||
| * Create a new lock instance. | ||
| */ | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = []) | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = [], int $heartbeat = 0) | ||
| { | ||
| parent::__construct($name, $seconds, $owner); | ||
| parent::__construct($name, $seconds, $owner, $heartbeat); | ||
|
|
||
| $container = ApplicationContext::getContainer(); | ||
| $cacheManager = $container->get(CacheManager::class); | ||
|
|
@@ -46,7 +46,7 @@ public function acquire(): bool | |
| return false; | ||
| } | ||
|
|
||
| return $this->store->set($this->name, $this->owner, $this->seconds); | ||
| return $this->store->set($this->name, $this->owner, $this->seconds) && $this->heartbeat(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -56,6 +56,7 @@ public function acquire(): bool | |
| public function release(): bool | ||
| { | ||
| if ($this->isOwnedByCurrentProcess()) { | ||
| $this->stopHeartbeat(); | ||
| return $this->store->delete($this->name); | ||
| } | ||
|
|
||
|
|
@@ -68,9 +69,19 @@ public function release(): bool | |
| #[Override] | ||
| public function forceRelease(): void | ||
| { | ||
| $this->stopHeartbeat(); | ||
| $this->store->delete($this->name); | ||
| } | ||
|
|
||
| #[Override] | ||
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, $this->seconds); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这里不对吧,一直在续期,正常到期了,其他也可以抢锁才对
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 防止任务还没有完成 , 就释放了锁
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 设计如此啊,如果不想提前释放,那就不设定时间就好了
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 有了心跳处理, seconds参数 重要性, 降低了; seconds 应该大于 heartbeat
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 那这个心跳就没有意义了
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
我感觉你的需求是mutex,不是lock
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 是lock, 动态增加锁的时间; 保证相同任务执行的唯一性 |
||
| } | ||
| return true; | ||
| } | ||
|
Comment on lines
+76
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delayExpiration() 缺少所有权验证。
建议在 protected function delayExpiration(): bool
{
if ($this->isOwnedByCurrentProcess() && $this->seconds > 0) {
return $this->store->set($this->name, $this->owner, $this->seconds);
}
return false;
}🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Returns the owner value written into the driver for this lock. | ||
| * @return string | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,9 +28,9 @@ class DatabaseLock extends AbstractLock | |
| /** | ||
| * Create a new lock instance. | ||
| */ | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = []) | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = [], int $heartbeat = 0) | ||
| { | ||
| parent::__construct($name, $seconds, $owner); | ||
| parent::__construct($name, $seconds, $owner, $heartbeat); | ||
|
|
||
| $constructor = array_merge(['pool' => 'default', 'table' => 'locks', 'prefix' => ''], $constructor); | ||
| if ($constructor['prefix']) { | ||
|
|
@@ -67,7 +67,7 @@ public function acquire(): bool | |
|
|
||
| $acquired = $updated >= 1; | ||
| } | ||
|
|
||
| $acquired && $this->heartbeat(); | ||
| return $acquired; | ||
| } | ||
|
|
||
|
|
@@ -78,6 +78,7 @@ public function acquire(): bool | |
| public function release(): bool | ||
| { | ||
| if ($this->isOwnedByCurrentProcess()) { | ||
| $this->stopHeartbeat(); | ||
| $this->connection->table($this->table) | ||
| ->where('key', $this->name) | ||
| ->where('owner', $this->owner) | ||
|
|
@@ -95,11 +96,29 @@ public function release(): bool | |
| #[Override] | ||
| public function forceRelease(): void | ||
| { | ||
| $this->stopHeartbeat(); | ||
| $this->connection->table($this->table) | ||
| ->where('key', $this->name) | ||
| ->delete(); | ||
| } | ||
|
|
||
| #[Override] | ||
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| $updated = $this->connection->table($this->table) | ||
| ->where('key', $this->name) | ||
| ->where(fn ($query) => $query->where('owner', $this->owner)->orWhere('expiration', '<=', time())) | ||
| ->update([ | ||
| 'owner' => $this->owner, | ||
| 'expiration' => $this->expiresAt(), | ||
| ]); | ||
|
|
||
| return $updated >= 1; | ||
| } | ||
| return true; | ||
| } | ||
|
Comment on lines
+105
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 修复潜在的锁竞争条件。 当前实现存在以下问题:
建议应用以下修复: #[Override]
protected function delayExpiration(): bool
{
if ($this->seconds > 0) {
$updated = $this->connection->table($this->table)
->where('key', $this->name)
- ->where(fn ($query) => $query->where('owner', $this->owner)->orWhere('expiration', '<=', time()))
+ ->where('owner', $this->owner)
->update([
- 'owner' => $this->owner,
'expiration' => $this->expiresAt(),
]);
return $updated >= 1;
}
return true;
}此外,考虑提取共享的更新逻辑以减少 🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Get the UNIX timestamp indicating when the lock should expire. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,9 +26,9 @@ class FileSystemLock extends AbstractLock | |
| /** | ||
| * Create a new lock instance. | ||
| */ | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = []) | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = [], int $heartbeat = 0) | ||
| { | ||
| parent::__construct($name, $seconds, $owner); | ||
| parent::__construct($name, $seconds, $owner, $heartbeat); | ||
|
|
||
| $constructor = array_merge(['config' => ['prefix' => 'lock:']], $constructor); | ||
| $this->store = make(FileSystemDriver::class, $constructor); | ||
|
|
@@ -44,7 +44,7 @@ public function acquire(): bool | |
| return false; | ||
| } | ||
|
|
||
| return $this->store->set($this->name, $this->owner, $this->seconds) == true; | ||
| return $this->store->set($this->name, $this->owner, $this->seconds) == true && $this->heartbeat(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -54,6 +54,7 @@ public function acquire(): bool | |
| public function release(): bool | ||
| { | ||
| if ($this->isOwnedByCurrentProcess()) { | ||
| $this->stopHeartbeat(); | ||
| return $this->store->delete($this->name); | ||
| } | ||
|
|
||
|
|
@@ -66,9 +67,19 @@ public function release(): bool | |
| #[Override] | ||
| public function forceRelease(): void | ||
| { | ||
| $this->stopHeartbeat(); | ||
| $this->store->delete($this->name); | ||
| } | ||
|
|
||
| #[Override] | ||
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, $this->seconds); | ||
| } | ||
| return true; | ||
| } | ||
|
Comment on lines
+74
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain验证父类在调用前是否检查锁所有权。
运行以下脚本验证父类是否在调用 🏁 Script executed: #!/bin/bash
# 描述:检查 AbstractLock 如何调用 delayExpiration() 以及是否进行所有权验证
# 查找 AbstractLock 中调用 delayExpiration 的位置及其上下文
ast-grep --pattern $'class AbstractLock {
$$$
delayExpiration()
$$$
}'
# 搜索 AbstractLock 中调用 delayExpiration 的代码上下文
rg -A 10 -B 5 'delayExpiration\(\)' src/lock/src/Driver/AbstractLock.phpLength of output: 771 需要更完整的上下文信息。我来获取 AbstractLock 中包含 Length of output: 1765 🏁 Script executed: #!/bin/bash
# Get the full method containing delayExpiration() call with extensive context
rg -B 30 'delayExpiration\(\)' src/lock/src/Driver/AbstractLock.phpLength of output: 1639 需要验证 FileSystemLock 的 Length of output: 539 在 验证确认了原始审查的合理性。 应在 protected function delayExpiration(): bool
{
if ($this->isOwnedByCurrentProcess() && $this->seconds > 0) {
return $this->store->set($this->name, $this->owner, $this->seconds);
}
return false;
}🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Returns the owner value written into the driver for this lock. | ||
| * @return string | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,9 +28,9 @@ class RedisLock extends AbstractLock | |
| /** | ||
| * Create a new lock instance. | ||
| */ | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = []) | ||
| public function __construct(string $name, int $seconds, ?string $owner = null, array $constructor = [], int $heartbeat = 0) | ||
| { | ||
| parent::__construct($name, $seconds, $owner); | ||
| parent::__construct($name, $seconds, $owner, $heartbeat); | ||
|
|
||
| $constructor = array_merge(['pool' => 'default', 'prefix' => ''], $constructor); | ||
| if ($constructor['prefix']) { | ||
|
|
@@ -46,7 +46,7 @@ public function __construct(string $name, int $seconds, ?string $owner = null, a | |
| public function acquire(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, ['NX', 'EX' => $this->seconds]) == true; | ||
| return $this->store->set($this->name, $this->owner, ['NX', 'EX' => $this->seconds]) == true && $this->heartbeat(); | ||
| } | ||
|
|
||
| return $this->store->setNX($this->name, $this->owner) === true; | ||
|
|
@@ -58,6 +58,7 @@ public function acquire(): bool | |
| #[Override] | ||
| public function release(): bool | ||
| { | ||
| $this->stopHeartbeat(); | ||
| return (bool) $this->store->eval(LuaScripts::releaseLock(), [$this->name, $this->owner], 1); | ||
| } | ||
|
|
||
|
|
@@ -67,9 +68,19 @@ public function release(): bool | |
| #[Override] | ||
| public function forceRelease(): void | ||
| { | ||
| $this->stopHeartbeat(); | ||
| $this->store->del($this->name); | ||
| } | ||
|
|
||
| #[Override] | ||
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, ['EX' => $this->seconds]); | ||
| } | ||
| return true; | ||
| } | ||
|
Comment on lines
+75
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delayExpiration() 缺少所有权验证。
建议使用类似 protected function delayExpiration(): bool
{
if ($this->seconds > 0) {
$script = <<<'LUA'
if redis.call("get", KEYS[1]) == ARGV[1] then
return redis.call("expire", KEYS[1], ARGV[2])
else
return 0
end
LUA;
return (bool) $this->store->eval($script, [$this->name, $this->owner, $this->seconds], 1);
}
return true;
} |
||
|
|
||
| /** | ||
| * Returns the owner value written into the driver for this lock. | ||
| * @return string | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
| /** | ||
| * This file is part of friendsofhyperf/components. | ||
| * | ||
| * @link https://github.com/friendsofhyperf/components | ||
| * @document https://github.com/friendsofhyperf/components/blob/main/README.md | ||
| * @contact huangdijia@gmail.com | ||
| */ | ||
|
|
||
| namespace FriendsOfHyperf\Lock\Exception; | ||
|
|
||
| use Exception; | ||
|
|
||
| class HeartbeatException extends Exception | ||
| { | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.