lock add heartbeat param #1030
lock add heartbeat param #1030
Conversation
Walkthrough为锁系统新增可选心跳（heartbeat）参数并在锁持有期间可启动协程周期性刷新过期时间。改动涉及注解、工厂、顶层函数、监听器、抽象驱动与各驱动实现，并新增 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as 客户端
participant LockFactory as LockFactory
participant Driver as 驱动实现
participant HB as 心跳协程
Client->>LockFactory: make(name, seconds, owner, driver, heartbeat)
LockFactory->>Driver: new Driver(..., heartbeat)
Client->>Driver: acquire()/block(callback)
activate Driver
Driver->>Driver: 获取锁成功
alt heartbeat > 0 且 seconds > 0
Driver->>HB: 启动心跳协程
activate HB
loop 每 heartbeat 秒
HB->>Driver: delayExpiration()
Driver-->>Driver: 刷新过期或返回不支持
end
deactivate HB
end
Driver->>Driver: 执行回调并释放锁
Driver->>Driver: stopHeartbeat() 停止心跳
deactivate Driver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 分钟 需要额外关注：
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.32)At least one path must be specified to analyse. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lock/src/Driver/DatabaseLock.php (1)
31-41: DatabaseLock：heartbeat 透传与续期逻辑合理，但需要修正代码风格
- 构造函数新增
$heartbeat并调用parent::__construct($name, $seconds, $owner, $heartbeat);，与其它驱动（如FileSystemLock）保持一致，这部分逻辑是正确的。delayExpiration()基本复用了acquire()中的更新条件：仅在当前 owner 或锁已过期的情况下续期，能有效避免误抢锁，语义上也合理。但本段代码存在会触发 php-cs-fixer 的风格问题，正好也和 CI 失败相吻合：
if ($this->seconds > 0){在{前缺少空格；update([...])中数组元素的缩进与本文件其余部分不一致。建议直接运行一次项目配置好的 fixer（例如
vendor/bin/php-cs-fixer fix），统一修复风格问题以通过当前流水线。Also applies to: 74-89
src/lock/src/Driver/CacheLock.php (1)
29-37: CacheLock：delayExpiration 缺少所有权校验且存在小的代码风格问题
- 构造函数扩展为
(..., array $constructor = [], int $heartbeat = 0)并传入parent::__construct(...)，与其它驱动保持一致，这一部分没有问题。delayExpiration()目前的实现：if ($this->seconds > 0){ return $this->store->set($this->name, $this->owner, $this->seconds); } return true;仅依赖
$this->seconds > 0就无条件set，没有确认当前进程仍然是锁的 owner。结合AbstractLock::heartbeat()的无限循环，如果旧的心跳协程还在运行，而锁已经被其它进程（新 owner）获取，这里可能会把 value 和 TTL 又覆盖回旧 owner，导致锁被“偷回”或长期不释放。建议：
#[Override] protected function delayExpiration(): bool { if ($this->seconds > 0 && $this->isOwnedByCurrentProcess()) { return $this->store->set($this->name, $this->owner, $this->seconds); } return true; }这样语义就和
DatabaseLock里通过 where 条件保证 owner/过期状态再续期的做法一致。此外，这里同样存在 CI 指出的风格问题：
if ($this->seconds > 0){花括号前应有一个空格。建议运行一次项目配置的 php-cs-fixer，统一修复风格以通过流水线。Also applies to: 52-59
🧹 Nitpick comments (7)
src/lock/src/Annotation/Lock.php (1)
20-26: Lock 注解新增 heartbeat 字段设计基本合理，建议约束取值范围
- 在构造函数末尾增加
public int $heartbeat = 0保持了向后兼容性，也和其它调用链保持了一致。- 目前在
AbstractLock::heartbeat()中是通过if ($this->heartbeat == 0 ...)判断是否禁用心跳，而不是<= 0。如果用户在注解上误填了负数，心跳协程仍会启动并向调度器传入负间隔，行为不太可控。建议：
- 要么在注解层约束
heartbeat为非负值（例如在使用说明中注明，并在运行时做一次max(0, $heartbeat)正规化）；- 要么在
AbstractLock::heartbeat()中改成if ($this->heartbeat <= 0 ...，统一将 0/负数都视为禁用心跳。当前实现可以工作，但对异常配置的防御性还可以稍微加强。
src/lock/src/Driver/RedisLock.php (1)
31-40: RedisLock 心跳续期实现可用，但可以提升所有权安全性优点：
- 构造函数新增
int $heartbeat = 0并传给parent::__construct(...)，与工厂注入的参数名一致，心跳参数贯通完整。delayExpiration()中根据seconds>0 时调用SET key value EX seconds实现续期，和acquire()的 TTL 语义匹配。需要考虑的点：
- 当前续期实现不再校验 owner，也没有使用 Lua 脚本做 CAS。极端情况下如果 TTL 已过期且其它客户端已经用新的 owner 成功
acquire()了锁，而心跳协程还在跑，那么一次delayExpiration()会直接用旧的$this->owner和新的 TTL 覆盖锁值，相当于“抢回”已经失去的锁。- 由于原本 TTL 到期后本地仍可能继续执行临界区，这个问题在没有心跳时也存在，但增加心跳后“抢回锁”的窗口理论上会放大。
建议（可选优化）：
- 参考
release()里使用的 Lua 脚本，增加一个“仅当当前 owner 匹配时才续期”的脚本，用于delayExpiration()；- 或者在
delayExpiration()中先读当前 owner，比对$this->owner不一致时直接返回 false 并在上层将heartbeat置 0，停止后续续期。另外，CI 报告 PHP-CS-Fixer 仍有文件可自动修复，合并前记得在本地跑一遍
php-cs-fixer保证风格一致。Also applies to: 55-62
src/lock/src/Driver/FileSystemLock.php (1)
29-35: FileSystemLock 心跳续期逻辑合理，顺便修一下代码风格逻辑方面：
- 构造函数新增
int $heartbeat = 0并传给parent::__construct(...)，和工厂的参数注入一致。delayExpiration()中在seconds > 0时调用$this->store->set($this->name, $this->owner, $this->seconds)续期，和acquire()写入的 TTL 一致，语义上没有问题。可以改进的点：
- 代码风格上
if ($this->seconds > 0){少了一个空格，会被 PHP-CS-Fixer 报出来，也是当前 CI 失败的原因之一，建议改为：- if ($this->seconds > 0){ + if ($this->seconds > 0) {
- 类似 Redis 驱动，如果你希望在 TTL 已过期且文件内容已经被其它进程改写后不再“抢回”锁，可以考虑在续期前通过
getCurrentOwner()比对 owner，不一致时直接返回 false 并停止后续心跳。整体实现已经能满足需求，上述修改主要是风格和健壮性的小优化。
Also applies to: 49-56
src/lock/src/Annotation/Blockable.php (1)
20-27: Blockable 注解新增 heartbeat 字段与整体设计一致，建议补充约束说明
- 构造函数末尾加上
public int $heartbeat = 0，和Lock注解及工厂、驱动层的参数顺序保持一致，默认值也保证了向后兼容。- 同样地，
heartbeat类型是int，目前没有显式限制其为非负数；结合AbstractLock::heartbeat()里对 0 的特殊处理，建议在使用说明或实现中约束heartbeat >= 0，避免误填负值导致心跳循环行为异常。如果后续在
AbstractLock层统一用<= 0作为“关闭心跳”的条件，这里的默认值和含义也会非常清晰。src/lock/src/Driver/CoroutineLock.php (1)
42-57: CoroutineLock 的 heartbeat 参数目前形同虚设，建议显式说明或支持续期
- 构造函数虽然接受
$heartbeat并传给AbstractLock，但delayExpiration()直接返回false，意味着即便调用方配置了 heartbeat，协程锁也不会延长$seconds对应的定时forceRelease()。- 相比 Redis/Cache/Database 等驱动会通过
delayExpiration()真正刷新过期时间，这里的行为是不一致的，容易让通过统一LockFactory使用锁的调用方误以为所有驱动对 heartbeat 支持一致。建议二选一处理以提升可预期性：
- 在类或方法注释中明确说明：
CoroutineLock不支持 heartbeat 续期，delayExpiration()永远返回false（当前行为即为 no-op）；- 或者在
delayExpiration()中重新设置self::$timer->after(...)并清理旧的timerId，使协程锁在 heartbeat 模式下也能真正推迟超时时间，与acquire()中的定时逻辑保持一致。Also applies to: 89-94
src/lock/src/Driver/AbstractLock.php (2)
39-45: 属性名与方法名同为 heartbeat，略显混淆同一个类中同时存在：
- 属性：
protected int $heartbeat = 0;- 方法：
private function heartbeat(): void虽然在 PHP 里这是合法的，但会在阅读和 IDE 自动补全时造成一定混淆。可以考虑做一个小改名提升可读性，例如：
- 属性改为
$heartbeatInterval；- 方法改为
startHeartbeat()或类似语义的名字。属于纯可读性优化，可按团队口味选择是否调整。
Also applies to: 161-166
161-181: heartbeat 循环中完全吞掉异常，建议至少记录告警日志在心跳协程中：
try { $this->delayExpiration(); } catch (Throwable $throwable) { // print log }目前只是留下了
// print log注释，实际运行时所有异常都会被静默吞掉。如果底层驱动的delayExpiration()持续抛错（例如数据库/缓存异常），可能导致锁无法续期且问题难以及时发现。建议引入框架内已有的日志组件，在这里至少打出 warning 级别日志，包含：
- 驱动类型；
- 锁名
$this->name；- owner / seconds / heartbeat 等关键上下文；
- 异常信息与堆栈。
这样一旦心跳续期出问题，可以通过日志快速定位。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/lock/src/Annotation/Blockable.php(1 hunks)src/lock/src/Annotation/BlockableAspect.php(1 hunks)src/lock/src/Annotation/Lock.php(1 hunks)src/lock/src/Driver/AbstractLock.php(6 hunks)src/lock/src/Driver/CacheLock.php(2 hunks)src/lock/src/Driver/CoroutineLock.php(2 hunks)src/lock/src/Driver/DatabaseLock.php(2 hunks)src/lock/src/Driver/FileSystemLock.php(2 hunks)src/lock/src/Driver/RedisLock.php(2 hunks)src/lock/src/Functions.php(1 hunks)src/lock/src/Listener/RegisterPropertyHandlerListener.php(1 hunks)src/lock/src/LockFactory.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/lock/src/Annotation/Lock.php (1)
src/lock/src/Driver/AbstractLock.php (1)
heartbeat(161-181)
src/lock/src/LockFactory.php (2)
src/lock/src/Driver/AbstractLock.php (2)
owner(119-122)heartbeat(161-181)src/lock/src/Driver/LockInterface.php (1)
owner(44-44)
src/lock/src/Functions.php (2)
src/lock/src/Driver/AbstractLock.php (2)
owner(119-122)heartbeat(161-181)src/lock/src/LockFactory.php (2)
LockFactory(21-51)make(30-50)
src/lock/src/Driver/FileSystemLock.php (1)
src/lock/src/Driver/AbstractLock.php (4)
__construct(44-48)owner(119-122)heartbeat(161-181)delayExpiration(146-146)
src/lock/src/Annotation/Blockable.php (1)
src/lock/src/Driver/AbstractLock.php (1)
heartbeat(161-181)
src/lock/src/Driver/RedisLock.php (1)
src/lock/src/Driver/AbstractLock.php (4)
__construct(44-48)owner(119-122)heartbeat(161-181)delayExpiration(146-146)
src/lock/src/Listener/RegisterPropertyHandlerListener.php (2)
src/lock/src/Driver/AbstractLock.php (2)
heartbeat(161-181)owner(119-122)src/lock/src/LockFactory.php (1)
make(30-50)
src/lock/src/Driver/CacheLock.php (2)
src/lock/src/Driver/CoroutineLock.php (5)
__construct(42-57)Override(62-87)Override(89-94)Override(100-108)Override(113-123)src/lock/src/Driver/RedisLock.php (5)
__construct(31-40)Override(45-53)Override(55-62)Override(67-71)Override(76-80)
src/lock/src/Driver/CoroutineLock.php (2)
src/lock/src/Driver/AbstractLock.php (4)
heartbeat(161-181)__construct(44-48)owner(119-122)delayExpiration(146-146)src/lock/src/Driver/FileSystemLock.php (1)
__construct(29-35)
src/lock/src/Driver/DatabaseLock.php (4)
src/lock/src/Driver/AbstractLock.php (6)
__construct(44-48)owner(119-122)heartbeat(161-181)Override(64-81)Override(87-114)delayExpiration(146-146)src/lock/src/Driver/CacheLock.php (5)
__construct(29-37)Override(42-50)Override(52-59)Override(65-73)Override(78-82)src/lock/src/Driver/FileSystemLock.php (3)
__construct(29-35)Override(40-48)Override(49-56)src/lock/src/Driver/RedisLock.php (5)
__construct(31-40)Override(45-53)Override(55-62)Override(67-71)Override(76-80)
src/lock/src/Annotation/BlockableAspect.php (2)
src/lock/src/LockFactory.php (1)
make(30-50)src/lock/src/Driver/AbstractLock.php (1)
heartbeat(161-181)
🪛 GitHub Actions: tests
src/lock/src/Driver/FileSystemLock.php
[error] 1-1: PHP CS Fixer: 5 files can be fixed. Dry-run reported diffs; the step failed with exit code 8. Command: vendor/bin/php-cs-fixer fix $1 --dry-run --diff --verbose.
src/lock/src/Driver/RedisLock.php
[error] 1-1: PHP CS Fixer: 5 files can be fixed. Dry-run reported diffs; the step failed with exit code 8. Command: vendor/bin/php-cs-fixer fix $1 --dry-run --diff --verbose.
src/lock/src/Driver/CacheLock.php
[error] 1-1: PHP CS Fixer: 5 files can be fixed. Dry-run reported diffs; the step failed with exit code 8. Command: vendor/bin/php-cs-fixer fix $1 --dry-run --diff --verbose.
src/lock/src/Driver/CoroutineLock.php
[error] 1-1: PHP CS Fixer: 5 files can be fixed. Dry-run reported diffs; the step failed with exit code 8. Command: vendor/bin/php-cs-fixer fix $1 --dry-run --diff --verbose.
src/lock/src/Driver/DatabaseLock.php
[error] 1-1: PHP CS Fixer: 5 files can be fixed. Dry-run reported diffs; the step failed with exit code 8. Command: vendor/bin/php-cs-fixer fix $1 --dry-run --diff --verbose.
🔇 Additional comments (4)
src/lock/src/Annotation/BlockableAspect.php (1)
42-43: Blockable 注解心跳参数透传正确这里通过命名参数把
heartbeat从注解传到LockFactory::make()，同时保持seconds仍然使用ttl，语义一致且不会破坏已有调用，看起来是合理的改动。src/lock/src/LockFactory.php (1)
30-49: 新增 heartbeat 透传总体合理，但请确认自定义驱动兼容性
LockFactory::make()新增尾部参数int $heartbeat = 0，对现有按位置/命名参数调用基本兼容，内部也把'heartbeat' => $heartbeat传给驱动构造函数，这样标准驱动的心跳链路是完整的。- 需要注意的是：如果用户有自定义
LockInterface实现且构造函数中没有$heartbeat参数，当前make()仍然会带上heartbeat这个命名参数，具体行为取决于 Hyperf DI 对“多余参数”的处理策略；若容器不忽略未知参数，可能在实例化自定义驱动时抛异常。建议：
- 在文档中显式注明：自定义驱动需要接受
int $heartbeat = 0参数；- 或确认/保证容器会安全忽略多余的
'heartbeat'参数。整体实现没问题，只是这个向下兼容点需要再核实一下。
src/lock/src/Listener/RegisterPropertyHandlerListener.php (1)
47-54: 属性注入路径上的 heartbeat 透传完整这里从
Lock注解读取$annotation->heartbeat并作为第五个参数传给LockFactory::make()，与函数式/注解 AOP 等其他创建路径保持了统一，逻辑上是闭环的，没有明显问题。src/lock/src/Functions.php (1)
20-28: 全局辅助函数 lock() 的心跳支持实现到位
- 新增
int $heartbeat = 0参数，并在$name非空时透传到$factory->make(...)，与工厂和驱动层改动保持一致。- 当
$name === null时仍返回LockFactory本身，对现有调用者无破坏性影响。整体设计清晰，可以满足通过辅助函数直接配置心跳的需求。
| 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; | ||
| } |
There was a problem hiding this comment.
心跳协程生命周期与锁所有权存在潜在问题
当前 heartbeat() 的行为是：
- 在
get()/block()成功获取锁后启动一个协程； - 协程循环调用
CoordinatorManager::until(Constants::WORKER_EXIT)->yield($this->heartbeat)，只有在：- 收到
WORKER_EXIT信号，或 $this->heartbeat被设为 0
时才会停止。
- 收到
但代码只在带回调的用法中（get($callback)、block($seconds, $callback) 的 finally）将 $this->heartbeat = 0，对于以下场景：
- 调用
get()/block()不传回调，只拿到布尔结果后手动release()； - 回调外部代码主动调用
release()/forceRelease()（即便不常见）；
心跳协程仍然会继续运行，即使锁已经被释放或被其它进程重新获取。结合各驱动的 delayExpiration() 实现，这会带来两个风险：
- 资源泄漏：旧协程一直挂着定时
yield循环，直到 worker 退出； - 所有权紊乱：对
CacheLock/RedisLock这类无条件set的实现，旧 owner 的心跳可能在锁已被新 owner 获取后继续覆盖 owner 和 TTL，造成“僵尸锁”或错误续期。
建议：
- 抽一个类似
private function stopHeartbeat(): void，内部把$this->heartbeat = 0;，并在：get()/block()的finally中调用；release()/forceRelease()中也调用，确保无论使用模式如何，释放时都能终止心跳协程。
- 在
heartbeat()循环内部增加所有权校验，例如：
if (! $this->isOwnedByCurrentProcess()) {
return;
}这样即使调用方忘记 stop，也能在丢失 owner 后自动停止续期，避免旧实例干扰新 owner。
这两点可以显著降低长时间运行任务下出现“锁被旧实例反复续期”的风险，更符合 PR 描述中对长任务锁健壮性的预期。
Also applies to: 69-76, 103-109, 161-181
🤖 Prompt for AI Agents
In src/lock/src/Driver/AbstractLock.php around lines 39-48 (also applies to
69-76, 103-109, 161-181), the heartbeat coroutine may continue running after the
lock is released causing resource leaks and stale owner renewals; add a private
stopHeartbeat(): void that sets $this->heartbeat = 0 and call it from the
finally blocks of get()/block() and from release()/forceRelease() so the
coroutine is stopped whenever the lock is released, and modify the heartbeat()
loop to check ownership each iteration (e.g. if not owned by current process
then exit) so a lost owner cannot continue renewing the lock even if
stopHeartbeat() was not invoked.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lock/src/Driver/FileSystemLock.php (1)
72-79: 考虑消除与 CacheLock 的代码重复。此方法的实现与
CacheLock::delayExpiration()完全相同。如果多个驱动都使用相同的逻辑，可以考虑将此实现提取到父类AbstractLock中（前提是 store 接口已标准化）。不过，由于不同驱动使用不同的 store 类型，当前的重复可能是有意为之的设计模式。运行以下脚本检查其他驱动是否也有相同的实现模式：
#!/bin/bash # 描述：检查所有驱动的 delayExpiration 实现是否相似 # 查找所有驱动中的 delayExpiration 方法实现 fd -e php -x rg -A 7 'function delayExpiration' {} \; src/lock/src/Driver/
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lock/src/Driver/CacheLock.php(2 hunks)src/lock/src/Driver/CoroutineLock.php(2 hunks)src/lock/src/Driver/DatabaseLock.php(2 hunks)src/lock/src/Driver/FileSystemLock.php(2 hunks)src/lock/src/Driver/RedisLock.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lock/src/Driver/RedisLock.php
- src/lock/src/Driver/CacheLock.php
- src/lock/src/Driver/CoroutineLock.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/lock/src/Driver/DatabaseLock.php (2)
src/lock/src/Driver/FileSystemLock.php (1)
__construct(29-35)src/lock/src/Driver/AbstractLock.php (4)
__construct(44-48)owner(119-122)heartbeat(161-181)delayExpiration(146-146)
src/lock/src/Driver/FileSystemLock.php (2)
src/lock/src/Driver/CacheLock.php (5)
__construct(29-37)Override(42-50)Override(55-63)Override(68-72)Override(74-81)src/lock/src/Driver/AbstractLock.php (4)
__construct(44-48)owner(119-122)heartbeat(161-181)delayExpiration(146-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
🔇 Additional comments (2)
src/lock/src/Driver/DatabaseLock.php (1)
31-33: 构造函数正确地添加了 heartbeat 参数。构造函数的修改与其他驱动（如 FileSystemLock）的模式一致，正确地接收 heartbeat 参数并传递给父类。
src/lock/src/Driver/FileSystemLock.php (1)
29-31: 构造函数签名更新正确。heartbeat 参数的添加符合预期，默认值为 0 表示默认禁用心跳机制，这是一个向后兼容的更改。参数顺序合理，可选参数放在最后。
| #[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; | ||
| } |
There was a problem hiding this comment.
修复潜在的锁竞争条件。
当前实现存在以下问题：
-
主要问题 - 锁所有权冲突：第 109 行的
orWhere('expiration', '<=', time())条件允许心跳在锁过期后重新声明锁。这可能导致竞争条件：- 进程 A 持有锁
- 锁过期
- 进程 B 获取锁
- 进程 A 的心跳运行并"窃取"锁
- 结果：两个进程都认为自己拥有锁，违反互斥保证
心跳的目的是延长当前持有的锁的过期时间，不应该在锁已过期后重新声明它。
-
必要重构 - 代码重复：第 107-113 行的更新逻辑与
acquire()方法的第 60-66 行几乎完全相同，应该提取为共享辅助方法。 -
次要问题 - 不必要的 owner 更新：第 111 行更新了
owner字段，但心跳只在进程已经拥有锁时运行，因此只需要更新expiration。
建议应用以下修复：
#[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;
}此外，考虑提取共享的更新逻辑以减少 acquire() 和 delayExpiration() 之间的代码重复。
🤖 Prompt for AI Agents
In src/lock/src/Driver/DatabaseLock.php around lines 103 to 118, the
delayExpiration() method currently allows updating the row when expiration <=
time(), which can let a heartbeat "steal" an expired lock and cause two
processes to believe they hold the lock; change the WHERE to only match the
current owner (remove the orWhere('expiration', '<=', time()) clause) so the
heartbeat only extends locks owned by this process, and update only the
expiration column (do not overwrite owner). Also refactor the update logic into
a shared private helper used by both acquire() and delayExpiration() to
eliminate duplication (move the query/update into one method that accepts the
matching where-clauses and the fields to update).
| #[Override] | ||
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, $this->seconds); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
验证父类在调用前是否检查锁所有权。
delayExpiration() 方法直接调用 $this->store->set() 来刷新锁的过期时间，但没有像 release() 方法（第56行）那样先验证当前进程是否拥有该锁。如果父类 AbstractLock 在调用此方法前没有进行所有权检查，可能会导致进程延长不属于自己的锁的过期时间。
运行以下脚本验证父类是否在调用 delayExpiration() 前检查所有权：
🏁 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 中包含 delayExpiration() 调用的完整方法：
Wait, I need to search the repository. Let me correct that:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full heartbeat method or enclosing function containing delayExpiration() call
rg -B 30 -A 5 'delayExpiration\(\)' src/lock/src/Driver/AbstractLock.php | head -80
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 的 release() 方法是否确实执行了所有权检查，以及其他驱动程序的实现模式：
---
🏁 Script executed:
```shell
#!/bin/bash
# Check FileSystemLock::release() for ownership verification
rg -B 5 -A 10 'public function release' src/lock/src/Driver/FileSystemLock.php
# Check if CacheLock has same delayExpiration() implementation
rg -B 3 -A 8 'protected function delayExpiration' src/lock/src/Driver/CacheLock.php
Length of output: 539
在 delayExpiration() 方法中添加所有权验证检查。
验证确认了原始审查的合理性。release() 方法通过 isOwnedByCurrentProcess() 检查所有权后才删除锁，而 delayExpiration() 方法直接调用 $this->store->set() 来延长锁的过期时间，没有进行任何所有权验证。这种不一致的模式使得代码可能允许进程延长不属于它的锁。
应在 delayExpiration() 方法中添加所有权检查，使其与 release() 方法的保护逻辑一致：
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
In src/lock/src/Driver/FileSystemLock.php around lines 72-79, delayExpiration()
currently extends a lock's TTL without verifying ownership; add a check so the
method only calls $this->store->set(...) when $this->isOwnedByCurrentProcess()
is true and $this->seconds > 0, and return false otherwise (i.e., do not extend
locks not owned by the current process).
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, $this->seconds); |
There was a problem hiding this comment.
这里不对吧，一直在续期，正常到期了，其他也可以抢锁才对
There was a problem hiding this comment.
设计如此啊，如果不想提前释放，那就不设定时间就好了
There was a problem hiding this comment.
有了心跳处理, seconds参数 重要性, 降低了;
seconds 应该大于 heartbeat
There was a problem hiding this comment.
有了心跳处理, seconds参数 重要性, 降低了;
seconds 应该大于 heartbeat
我感觉你的需求是mutex，不是lock
There was a problem hiding this comment.
是lock, 动态增加锁的时间; 保证相同任务执行的唯一性
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/lock/src/Driver/FileSystemLock.php (1)
74-81: delayExpiration() 缺少所有权验证。与之前的审查评论一致，
delayExpiration()方法直接调用$this->store->set()来延长锁的过期时间，但没有进行任何所有权验证。这可能导致进程延长不属于它的锁。应在
delayExpiration()方法中添加所有权检查：protected function delayExpiration(): bool { if ($this->isOwnedByCurrentProcess() && $this->seconds > 0) { return $this->store->set($this->name, $this->owner, $this->seconds); } return false; }src/lock/src/Driver/DatabaseLock.php (1)
105-120: 修复潜在的锁竞争条件和代码重复。与之前的审查评论一致，当前实现存在以下问题：
锁所有权冲突：第 111 行的
orWhere('expiration', '<=', time())允许心跳在锁过期后重新声明锁，可能导致两个进程同时认为自己拥有锁。心跳的目的是延长当前持有的锁的过期时间，不应该在锁已过期后重新获取它。不必要的 owner 更新：第 113 行更新了
owner字段，但心跳只在进程已经拥有锁时运行，因此只需要更新expiration。代码重复：第 109-115 行的更新逻辑与
acquire()方法的第 60-66 行几乎完全相同。建议应用以下修复：
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; }src/lock/src/Driver/AbstractLock.php (1)
156-177: 心跳循环中缺少所有权验证。当前
heartbeat()实现在协程循环中定期调用delayExpiration()，但没有在循环内检查当前进程是否仍然拥有该锁。虽然stopHeartbeat()方法可以终止心跳，但在以下场景中仍存在风险：
- 如果锁因超时自动过期并被其他进程获取，旧的心跳协程可能在检测到
$this->heartbeat = 0之前继续尝试刷新过期时间- 结合各驱动的
delayExpiration()实现（特别是那些不检查所有权的），可能导致旧进程意外延长新进程的锁建议在心跳循环内添加所有权检查：
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) { // print log } } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lock/src/Driver/AbstractLock.php(4 hunks)src/lock/src/Driver/CacheLock.php(4 hunks)src/lock/src/Driver/DatabaseLock.php(4 hunks)src/lock/src/Driver/FileSystemLock.php(4 hunks)src/lock/src/Driver/RedisLock.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/lock/src/Driver/RedisLock.php (1)
src/lock/src/Driver/AbstractLock.php (5)
__construct(44-48)owner(114-117)heartbeat(156-177)stopHeartbeat(179-182)delayExpiration(141-141)
src/lock/src/Driver/CacheLock.php (1)
src/lock/src/Driver/AbstractLock.php (5)
__construct(44-48)owner(114-117)heartbeat(156-177)stopHeartbeat(179-182)delayExpiration(141-141)
src/lock/src/Driver/FileSystemLock.php (1)
src/lock/src/Driver/AbstractLock.php (5)
__construct(44-48)owner(114-117)heartbeat(156-177)stopHeartbeat(179-182)delayExpiration(141-141)
src/lock/src/Driver/DatabaseLock.php (2)
src/lock/src/Driver/AbstractLock.php (5)
__construct(44-48)owner(114-117)heartbeat(156-177)stopHeartbeat(179-182)delayExpiration(141-141)src/lock/src/Driver/LockInterface.php (1)
owner(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
🔇 Additional comments (16)
src/lock/src/Driver/RedisLock.php (3)
31-33: 构造函数正确传递心跳参数。构造函数签名已扩展以接受
heartbeat参数，并正确地将其传递给父类构造函数。这与整个 PR 中的心跳机制集成良好。
49-49: 正确集成心跳机制到锁获取流程。在成功获取锁后调用
$this->heartbeat()是正确的做法，确保只有在成功获取锁的情况下才启动心跳协程。
61-61: 正确在释放锁时停止心跳。在
release()和forceRelease()中调用stopHeartbeat()确保心跳协程在锁释放时被正确终止，避免资源泄漏。Also applies to: 71-71
src/lock/src/Driver/FileSystemLock.php (3)
29-31: 构造函数正确传递心跳参数。构造函数签名已扩展以接受
heartbeat参数，并正确地将其传递给父类构造函数。
47-47: 正确集成心跳机制到锁获取流程。在成功设置锁后调用
$this->heartbeat()确保只有在成功获取锁的情况下才启动心跳协程。
57-57: 正确在释放锁时停止心跳。在
release()和forceRelease()中调用stopHeartbeat()确保心跳协程在锁释放时被正确终止。Also applies to: 70-70
src/lock/src/Driver/CacheLock.php (3)
29-31: 构造函数正确传递心跳参数。构造函数签名已扩展以接受
heartbeat参数，并正确地将其传递给父类构造函数。
49-49: 正确集成心跳机制到锁获取流程。在成功设置锁后调用
$this->heartbeat()确保只有在成功获取锁的情况下才启动心跳协程。
59-59: 正确在释放锁时停止心跳。在
release()和forceRelease()中调用stopHeartbeat()确保心跳协程在锁释放时被正确终止。Also applies to: 72-72
src/lock/src/Driver/DatabaseLock.php (3)
31-33: 构造函数正确传递心跳参数。构造函数签名已扩展以接受
heartbeat参数，并正确地将其传递给父类构造函数。
70-71: 正确集成心跳机制到锁获取流程。在成功获取锁后调用
$this->heartbeat()确保只有在成功获取锁的情况下才启动心跳协程。注意第 70 行使用了语句级别的&&操作符来调用心跳。
81-81: 正确在释放锁时停止心跳。在
release()和forceRelease()中调用stopHeartbeat()确保心跳协程在锁释放时被正确终止。Also applies to: 99-99
src/lock/src/Driver/AbstractLock.php (4)
15-21: 正确添加心跳机制所需的导入。新增的导入（
Constants,CoordinatorManager,Coroutine,Throwable）是实现心跳机制所必需的。
39-48: 构造函数正确初始化心跳参数。构造函数签名已扩展以接受
heartbeat参数，并正确地初始化了$this->heartbeat属性。这为所有驱动提供了统一的心跳支持基础。
141-141: 正确定义抽象方法以支持心跳延期。新增的抽象方法
delayExpiration(): bool为各个驱动提供了统一的接口来实现锁过期时间的刷新逻辑。
179-182: stopHeartbeat() 方法正确实现。
stopHeartbeat()方法通过将$this->heartbeat设置为 0 来终止心跳协程。这与心跳循环中的检查（第 166 行）配合良好，确保协程能够及时退出。
| #[Override] | ||
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, $this->seconds); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
delayExpiration() 缺少所有权验证。
delayExpiration() 方法直接调用 $this->store->set() 来刷新过期时间，但没有验证当前进程是否拥有该锁。这与 release() 方法（第 58 行先检查 isOwnedByCurrentProcess() 再删除）形成对比。如果心跳协程在锁已被其他进程获取后继续运行，可能会导致进程延长不属于自己的锁的过期时间。
建议在 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
In src/lock/src/Driver/CacheLock.php around lines 76 to 83, delayExpiration()
currently refreshes the TTL unconditionally which can extend locks owned by
other processes; modify it to first verify ownership with
isOwnedByCurrentProcess() and only call $this->store->set($this->name,
$this->owner, $this->seconds) when both the lock is owned by the current process
and $this->seconds > 0, otherwise return false to avoid extending foreign locks.
| #[Override] | ||
| protected function delayExpiration(): bool | ||
| { | ||
| if ($this->seconds > 0) { | ||
| return $this->store->set($this->name, $this->owner, ['EX' => $this->seconds]); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
delayExpiration() 缺少所有权验证。
delayExpiration() 方法直接调用 $this->store->set() 刷新过期时间，但没有验证当前进程是否拥有该锁。这与 release() 方法（第 62 行使用 Lua 脚本进行原子性的所有权检查和删除）形成对比。如果心跳协程在锁已被其他进程获取后继续运行，可能会导致进程延长不属于自己的锁的过期时间。
建议使用类似 release() 的 Lua 脚本方式来原子性地验证所有权并刷新过期时间：
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;
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lock/src/Exception/HeartbeatException.php (1)
16-18: HeartbeatException 定义简单清晰，可考虑后续挂到统一异常层级当前直接继承
\Exception能满足需求，命名也直观。如果后续项目里有统一的锁异常基类（例如LockException一类），可以再将HeartbeatException调整为继承该基类，便于上层统一捕获锁相关异常。tests/Lock/AbstractLockTest.php (1)
62-66: TestLock 中 delayExpiration 实现足以满足抽象约束，可考虑增强对心跳行为的覆盖当前直接返回
true，用于满足AbstractLock::delayExpiration()抽象签名是合理的。不过既然本 PR 新增了心跳逻辑，后续可以考虑在测试中通过自增计数等方式，实现一个可观察的delayExpiration()，以便验证 heartbeat 协程是否按预期周期性调用该方法。src/lock/src/Driver/AbstractLock.php (2)
145-151: heartbeat() 仅在子类显式调用时生效，心跳生命周期容易在各驱动中实现不一致
AbstractLock新增了：
abstract protected function delayExpiration(): bool;protected function heartbeat(): boolprotected function stopHeartbeat(): void但从当前文件来看：
get()/block()/isOwnedByCurrentProcess()等公共逻辑内部都没有调用heartbeat()或stopHeartbeat()；- 因此即便上层通过工厂/注解传入了
heartbeat参数，如果某个驱动在acquire()成功后忘记调用heartbeat()，则该驱动的心跳功能实际上是关闭的；- 同样，如果释放逻辑（
release()/forceRelease()或回调finally）没有在合适位置调用stopHeartbeat()或确保getCurrentOwner()立刻反映 owner 变更，心跳协程可能比预期多跑一个周期。为了减少使用/实现层面的差异，建议考虑：
- 在抽象层统一触发心跳
- 例如：在
get()/block()中，当acquire()第一次返回true时调用一次$this->heartbeat()，避免每个驱动手动记忆调用。- 在统一路径上终止心跳
- 如果条件允许，可以在释放逻辑的公共实现（如增加非抽象的
protected function doRelease()）或get()/block()的finally中调用stopHeartbeat()，让“释放锁”与“停止心跳”绑定在一起。- 如果出于兼容性等原因必须由各驱动自主管理心跳，建议在抽象类的 PHPDoc 中清晰注明使用约定（何时应该调用
heartbeat()/stopHeartbeat()），以降低误用风险。另外，目前
delayExpiration()的返回值在heartbeat()中并未使用，如果该返回值没有业务语义，长期来看可以考虑改为void以减少误导。Also applies to: 160-189
176-181: delayExpiration() 抛出的异常被完全吞掉，建议至少记录日志或打标在心跳续期过程中：
try { $this->delayExpiration(); } catch (Throwable $throwable) { // print log }这里把所有
Throwable静默吞掉，只留了一条注释。如果驱动端发生异常（例如 Redis/缓存后端短暂故障），会导致锁续期失败、TTL 提前失效，但调用方几乎没有信号可以定位问题。建议至少：
- 使用 Hyperf 的日志组件或注入的 Logger 记录
$throwable（含 stack trace）；或者- 在注释中标记 TODO，并在后续迭代中补上统一的日志策略。
这样可以在不影响现有行为的前提下，提升故障可观测性。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lock/src/Driver/AbstractLock.php(4 hunks)src/lock/src/Exception/HeartbeatException.php(1 hunks)tests/Lock/AbstractLockTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Lock/AbstractLockTest.php (1)
src/lock/src/Driver/AbstractLock.php (1)
delayExpiration(145-145)
src/lock/src/Driver/AbstractLock.php (2)
src/lock/src/Exception/HeartbeatException.php (1)
HeartbeatException(16-18)tests/Lock/AbstractLockTest.php (1)
delayExpiration(63-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lock/src/Driver/AbstractLock.php (1)
69-82: 心跳功能未被启动，且协程未在释放时停止发现两个严重问题：
心跳从未启动：在成功获取锁之后（第 71 行），代码没有调用
$this->heartbeat()来启动心跳协程。这意味着整个心跳功能实际上不会生效。协程泄漏风险：尽管
stopHeartbeat()方法已实现，但finally块中没有调用它。如之前的评审所指出，这会导致心跳协程在锁释放后继续运行，造成资源泄漏和潜在的所有权混乱。建议修改如下：
public function get(?callable $callback = null) { $result = $this->acquire(); + if ($result) { + $this->heartbeat(); + } if ($result && is_callable($callback)) { try { return $callback(); } finally { + $this->stopHeartbeat(); $this->release(); } } return $result; }注意：对于不使用回调的场景（只返回布尔值），调用者需要手动调用
release()，建议在release()方法中也调用stopHeartbeat()。
♻️ Duplicate comments (2)
src/lock/src/Driver/AbstractLock.php (2)
89-113:block()方法存在与get()相同的心跳问题此方法也存在两个关键缺陷：
- 成功获取锁后（第 94-102 行循环结束后）未调用
$this->heartbeat()启动心跳finally块中未调用$this->stopHeartbeat()清理协程建议应用相同的修复：
public function block(int $seconds, ?callable $callback = null) { $starting = ((int) now()->format('Uu')) / 1000; $milliseconds = $seconds * 1000; 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)) { try { return $callback(); } finally { + $this->stopHeartbeat(); $this->release(); } } return true; }
45-51: 应在构造函数中显式拒绝负数心跳值当前的校验逻辑（第 49 行）仅在
$heartbeat > 0时检查其与$seconds的关系，允许负数心跳值悄无声息地通过构造函数。虽然第 162 行的守卫条件会阻止负数心跳启动，但这种隐式行为可能导致配置错误被静默忽略，增加调试难度。建议在构造函数中添加明确的校验：
public function __construct(protected string $name, protected int $seconds, ?string $owner = null, int $heartbeat = 0) { $this->owner = $owner ?? Str::random(); + if ($heartbeat < 0) { + throw new HeartbeatException('Heartbeat must be greater than or equal to 0.'); + } $this->heartbeat = $heartbeat; if ($seconds > 0 && $heartbeat > 0 && $seconds <= $heartbeat) { throw new HeartbeatException('Heartbeat must be less than lock seconds.'); } }这样可以在配置阶段就发现问题，而不是让负数值被静默禁用。基于之前的评审意见，这一校验有助于防止隐蔽的运行时问题。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lock/src/Driver/AbstractLock.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lock/src/Driver/AbstractLock.php (2)
src/lock/src/Exception/HeartbeatException.php (1)
HeartbeatException(16-18)tests/Lock/AbstractLockTest.php (1)
delayExpiration(63-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
🔇 Additional comments (3)
src/lock/src/Driver/AbstractLock.php (3)
14-22: 导入语句正确且必要新增的导入项都在代码中被正确使用，包括心跳异常、协程管理器和相关组件。
145-145: 抽象方法定义合理新增的
delayExpiration()抽象方法为各个驱动提供了刷新锁过期时间的扩展点，方法签名清晰明确。测试代码中已有简单实现返回true。
186-189:stopHeartbeat()实现正确方法实现简洁有效：将
$this->heartbeat设为 0 会触发心跳循环的退出条件（第 170 行）。虽然方法本身没有问题，但需要在适当的位置调用它（已在get()和block()方法的评审中指出）。
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lock/src/Driver/AbstractLock.php (1)
145-146:delayExpiration()返回值未被使用，心跳循环异常处理仍缺少实际日志
关于
delayExpiration()的返回值：
- 在
AbstractLock内部，heartbeat()调用delayExpiration()时并未使用其bool返回值；- 如果各具体驱动里也没有依赖这个返回值，建议考虑将签名简化为
void，或者在这里根据返回值做一些显式处理（例如false时直接退出心跳协程），以免接口含义模糊。- 这属于签名层面的轻量重构，可按需要权衡是否调整。
关于
heartbeat()中的异常处理（Line 176–180）：
- 当前仍然是
catch (Throwable $throwable) { // print log }，会完全吞掉delayExpiration()抛出的异常；- 对排查“锁续期失败”这类问题非常不友好，而且这点在之前评审中已经提到过。
建议至少增加一条告警级别的日志（示例）：
- try { - $this->delayExpiration(); - } catch (Throwable $throwable) { - // print log - } + try { + $this->delayExpiration(); + } catch (Throwable $throwable) { + // TODO: 按项目约定替换为实际的日志组件 + logger()->warning('Failed to delay lock expiration', [ + 'lock' => $this->name, + 'owner' => $this->owner, + 'exception' => $throwable->getMessage(), + ]); + }只要保证不会重新抛出异常、心跳循环能继续即可。
Also applies to: 160-184
🧹 Nitpick comments (2)
src/lock/src/Driver/AbstractLock.php (2)
40-51: 构造函数中 heartbeat 的约束整体合理，符合预期语义
protected int $heartbeat = 0作为“默认关闭心跳”很直观；if ($seconds > 0 && $heartbeat > 0 && $seconds <= $heartbeat)的校验能防止心跳间隔大于或等于锁过期时间，避免“刚续期就过期”的配置错误。需要注意的一点是：当
$seconds <= 0但$heartbeat > 0时不会抛异常，而是后续在heartbeat()的守卫条件中被视为“无效心跳”（直接返回 true，不开启协程）。如果你们希望这类配置在构造阶段就显式失败，可以考虑再补一条约束（例如$seconds <= 0 && $heartbeat > 0时抛HeartbeatException），否则当前行为也可以接受为“自动禁用心跳”的语义。
186-189:stopHeartbeat()仅修改标志位，需确保在具体驱动中正确调用当前
stopHeartbeat()只是简单地将$this->heartbeat置为 0，并依赖心跳协程下一轮循环里的：
if ($this->heartbeat == 0) return;- 加上
isOwnedByCurrentProcess()校验来实现“尽快退出心跳”的效果，这个思路是 OK 的。不过在本类中尚未看到对
stopHeartbeat()的调用，因此有两点需要确认：
- 各具体驱动的
release()/forceRelease()是否都会在真正释放锁之前调用stopHeartbeat()？- 是否需要在一些失败分支（例如获取锁失败但已经开启了心跳的极端情况）也调用
stopHeartbeat()以避免多余的协程长期挂着？如果这些调用已经在驱动里完成，那当前实现就比较完整；否则建议补充调用点，保证“锁不再被当前实例持有时，心跳协程能及时停掉”这一约束在所有路径上都成立。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lock/src/Driver/AbstractLock.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lock/src/Driver/AbstractLock.php (2)
src/lock/src/Exception/HeartbeatException.php (1)
HeartbeatException(16-18)src/lock/src/Driver/RedisLock.php (1)
__construct(31-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
🔇 Additional comments (1)
src/lock/src/Driver/AbstractLock.php (1)
14-22: 新引入依赖与异常类型与心跳特性一致，接口使用看起来合理
HeartbeatException、CoordinatorManager、Constants、Coroutine和Throwable的引入都在后文被正确使用，目前没有看到多余或错误的依赖引用。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Lock/FunctionsTest.php (1)
28-28: 正确更新了 mock 期望以包含心跳参数。这三处修改都正确地在
LockFactory::make()的 mock 期望中添加了第五个参数（心跳值 0），与新的方法签名保持一致。测试验证了当未显式传递心跳参数时，默认值 0 被正确传递。然而，当前测试套件缺少对非默认心跳值的覆盖。建议添加测试用例验证显式传递心跳参数的场景。
可以添加如下测试用例以提升覆盖率：
test('lock function passes custom heartbeat parameter', function () { $lockInstance = $this->mock(LockInterface::class); $factory = $this->mock(LockFactory::class, function ($mock) use ($lockInstance) { $mock->shouldReceive('make')->with('test', 60, null, 'default', 10)->andReturn($lockInstance); }); $this->instance(LockFactory::class, $factory); $result = lock('test', 60, null, 'default', 10); expect($result)->toBe($lockInstance); });Also applies to: 40-40, 52-52
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Lock/FunctionsTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Lock/FunctionsTest.php (1)
tests/Concerns/InteractsWithContainer.php (1)
mock(60-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lock/src/Driver/AbstractLock.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lock/src/Driver/AbstractLock.php (2)
src/lock/src/Exception/HeartbeatException.php (1)
HeartbeatException(16-18)tests/Lock/AbstractLockTest.php (1)
delayExpiration(63-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
🔇 Additional comments (4)
src/lock/src/Driver/AbstractLock.php (4)
14-24: 导入语句合理，支持心跳功能所需新增的导入项正确支持了心跳机制：异常类、应用容器、日志接口、协程协调器及工具类，均在代码中被使用。
42-42: 属性声明正确，默认值合理
$heartbeat属性默认为 0，表示心跳功能默认关闭，保持向后兼容性。
47-54: 构造函数验证逻辑正确，妥善处理心跳参数构造函数新增的
heartbeat参数及其验证逻辑合理：
- 要求
heartbeat < seconds，确保至少有一次续期机会在锁过期之前- 默认值 0 表示禁用心跳，保持向后兼容
- 验证条件正确处理了负数和边界情况
147-147: 抽象方法定义恰当，为具体驱动预留扩展点
delayExpiration()抽象方法为各驱动实现续期逻辑提供了统一接口，返回类型bool允许驱动报告操作成功与否。
| 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; | ||
| } |
There was a problem hiding this comment.
关键缺陷：心跳方法未被调用，功能无法生效
heartbeat() 方法的实现逻辑本身是正确的：
- 守卫条件（第 164 行）正确检查了有效性
- 所有权检查（第 175-177 行）防止了旧实例干扰新所有者
- 异常处理和日志记录（第 181 行）已正确实现
但存在关键问题：此方法在整个抽象类中从未被调用。
查看 get() 方法（第 71-84 行）和 block() 方法（第 91-115 行），在成功获取锁后都没有调用 heartbeat()，导致心跳机制根本不会启动。
建议在以下位置之一调用 heartbeat()：
- 在
get()方法中，第 73 行$result = $this->acquire();之后，当$result为 true 时调用 - 在
block()方法中，第 104 行成功获取锁后立即调用 - 或者在各个具体驱动的
acquire()方法成功返回前调用
示例修改（方案 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)) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lock/src/Driver/AbstractLock.php around lines 162-186: the heartbeat()
method is never invoked so the background renewal loop never starts; modify the
class to call $this->heartbeat() immediately after a successful lock acquisition
— e.g. in get() right after $result = $this->acquire(); when $result is true,
and likewise in block() immediately after a successful acquire/when the lock is
obtained (or ensure concrete drivers invoke heartbeat() before returning success
from acquire()); keep the existing guards and ownership checks as-is and ensure
the call is only made when acquisition succeeds.
| protected function stopHeartbeat(): void | ||
| { | ||
| $this->heartbeat = 0; | ||
| } |
There was a problem hiding this comment.
缺少 stopHeartbeat() 调用，心跳协程可能无法及时停止
stopHeartbeat() 方法的实现正确，但在可见代码中从未被调用：
get()方法的finally块（第 79 行）只调用了release()block()方法的finally块（第 110 行）也只调用了release()
根据之前的评审意见和最佳实践，应在释放锁时停止心跳协程，以防止资源泄漏。
建议在 finally 块中释放锁之前调用 stopHeartbeat()：
if ($result && is_callable($callback)) {
try {
return $callback();
} finally {
+ $this->stopHeartbeat();
$this->release();
}
}同样的修改也应应用于 block() 方法的 finally 块（第 110 行）。
此外，release() 和 forceRelease() 方法（由具体驱动实现）中也应该调用 stopHeartbeat()，以确保无论哪种释放路径都能停止心跳。
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lock/src/Driver/AbstractLock.php around lines 188-191: stopHeartbeat() is
implemented but never invoked; update the finally blocks in get() (around line
79) and block() (around line 110) to call $this->stopHeartbeat() before calling
$this->release(); also update the concrete driver implementations of release()
and forceRelease() (or their overrides) to invoke $this->stopHeartbeat() as part
of their release path so the heartbeat coroutine is always stopped regardless of
which release method is used.
|
这个放到lock组件 是不是更合适 |
mutex 还没有找到相对好点的规范，所以还没做成组件 |
|
看看这个PR：#1031 $lock = lock('lock_name', $seconds = 120);
if ($lock->get()) {
(new Timer)->tick(1, function() use ($lock) {
if (CoordinatorManager::until(Constants::WORKER_EXIT)->yield(Constants::WORKER_EXIT)) {
return Timer::STOP;
}
if (!$lock->refresh()) return Timer::STOP;
});
// do something
$lock->release();
} |
2 participants
在长时间执行任务时 加锁, 时间不好把控;
1: 设置加锁时间太长, 任务意外关闭, 重新启动任务时 锁没有释放, 任务一直获取不到锁
2: 增加心跳设置后, 保证锁的健壮性
3: 长时间的任务加锁, 避免较短的过期时间导致的锁过期
Summary by CodeRabbit
新功能
文档 / 错误处理
测试
✏️ Tip: You can customize this high-level summary in your review settings.