fix(database): 修复玩家数据并发写入一致性#710
Conversation
通过复合唯一索引、兼容迁移和原子 upsert 阻止重复记录,并收敛缓存延迟写与调度竞态以确保最新值最终落库。
FxRayHughes
left a comment
There was a problem hiding this comment.
Code Review — #710 fix(database): 修复玩家数据并发写入一致性
先说结论:(user, key) 复合唯一约束 + revision/ready/running 状态机是真实且必要的修复,旧 set() 的「先查后插」竞态确实会产生重复行,旧 drainWrites 缺失也确实会让旧快照覆盖新值。按列定义校验索引(而非只比名字)这一点做得尤其到位。
但有两处需要在合并前处理:一个是 setDelayed 修对之后新引入的数据丢失窗口,另一个是与 #709 在同一批文件上的冲突。
审阅方式:读 patch + 逐条对照仓库源码验证。未实跑 gradle 测试,下列结论均基于源码推导,已注明依据位置。
🔴 问题 1 — setDelayed 的延迟真正生效后,容器释放与关服会丢弃未落库的写入
事实基础
旧代码(DataContainer.kt:68-71、:129-134):
updateMap[key] = System.currentTimeMillis() - timeUnit.toMillis(delay) // ← 减
...
updateMap.filterValues { it < System.currentTimeMillis() } // ← 恒为 true写入的是过去的时间戳,因此 checkUpdate 的判断恒真。也就是说旧实现里 delay 参数从未生效,setDelayed 实际等价于「下一 tick 落库」,窗口 ≤ 1 秒(@Schedule(period = 20))。
本 PR 改为 deadlineAfter(...) 即 now + delay,延迟第一次真正生效——这个方向是对的,delayed value is not persisted before its deadline 测试也把它固化了下来。
问题在于落库时机变晚之后,没有任何路径兜底
checkUpdate 只遍历 playerDataContainer 里的容器(DataContainer.kt:154-157),而释放容器的两个入口都是直接 remove,不做 flush:
fun UUID.releasePlayerDataContainer() { playerDataContainer.remove(this) } // DatabaseHandler.kt:196
fun ProxyPlayer.releaseDataContainer() { playerDataContainer.remove(uniqueId) } // :203PlayerDatabase.releaseDataContainer(PlayerDatabase.kt:109-120)同样只 remove。整个 database-player 模块也没有 @Awake(LifeCycle.DISABLE) 钩子——我 grep 过,只有 AutoDataContainer.kt:106 一个 @Awake(LifeCycle.ACTIVE)。
触发场景
| 步骤 | 旧行为 | 新行为 |
|---|---|---|
插件调用 setDelayed("coins", 100)(默认 delay=3s) |
deadline 已是过去时间 | deadline = now + 3s |
玩家 1 秒后退出,触发 releaseDataContainer |
下一 tick 已落库,容器移除无影响 | 容器被移出 playerDataContainer |
| 之后 | 数据已在库里 | checkUpdate 再也遍历不到该容器,写入永久丢失 |
关服同理:DISABLE 阶段没有 flush,所有还在 deadline 内的延迟写全部丢弃。
严重性在于,这个丢失窗口是本 PR 引入的。旧代码因为延迟逻辑写坏了,反而不存在这个问题。PR 描述里「避免数据回档和丢失更新」的目标,在 setDelayed 这条路径上被反向影响了。
建议
- 在
DataContainer上加fun flush():把所有writeStates中deadline != null的项立即置为ready = true并同步排空(或直接同步写库,因为释放/关服时不应再依赖异步执行器)。 releasePlayerDataContainer/releaseDataContainer/PlayerDatabase.releaseDataContainer在remove之前先调flush()。- 加
@Awake(LifeCycle.DISABLE),对playerDataContainer全量flush()。注意此时调度器可能已拒绝任务,flush必须走同步路径而不是asyncExecutor。
顺带一提,setDelayed 的行为变更本身也值得写进兼容性说明:依赖旧「立即落库」语义的插件,升级后会看到落库延后 delay。
🔴 问题 2 — 与 #709 在同一批文件上冲突,且合并后会绕过 #709 的资源保护
#709 与本 PR 同时修改 database-player/build.gradle.kts 与 expansion/Database.kt,并且各自新增了完全相同的两个 Hikari 测试桩(test/kotlin/com/zaxxer/hikari_4_0_3/HikariConfig.kt、HikariDataSource.kt,内容逐字一致)。两者都从同一 base 分出,git 层面必然冲突。
build.gradle.kts 两边改的是同一处文件末尾(且原文件无换行符结尾):
合并时需要取并集,漏掉任一侧都会让对方的测试编译不过。
更值得注意的是语义冲突。 #709 把 Database.init 改成建表失败即关闭自有 DataSource:
init {
try {
type.tableVar().createTable(dataSource)
} catch (ex: Throwable) {
close() // ← 失败时释放自有连接池
throw ex
}
}本 PR 的 init 是:
init {
table.createTable(dataSource)
ensureUniqueKeyIndex() // ← 新增
}若按 #709 在前、#710 在后的顺序机械合并,ensureUniqueKeyIndex() 很容易落在 try 块之外。而它恰恰是最可能抛异常的一步(重复数据无法归并、索引名分配失败、跨节点迁移冲突),一旦抛出就绕过 #709 刚建立的「构造失败必释放」契约,泄漏连接池。
建议:明确两个 PR 的合并顺序,后合并的一方把 ensureUniqueKeyIndex() 放进 try 内,并在 PR 描述里标注对另一方的依赖。#720 作为整合 PR 时也需要复核这一点。
🟡 问题 3 — save(key) 由抛 NPE 变为静默删除数据库行
旧实现(DataContainer.kt:115-117):
fun save(key: String) {
submitAsync { database[user, key] = source[key]!! } // source 无此 key → NPE
}新实现把 state.value = source[key](可空)交给 drainWrites,而 drainWrites 对 null 的处理是 database.remove(user, key)。
于是 save("someKey") 在 source 不含该键时,从抛异常变成删库里的行。source 只是构造时的一次快照(val source = database[user]),下列情况会出现「库里有、source 里没有」:
- 另一节点或另一插件通过
forcedSet(sync = false)写入了该键 - 插件先
keys()拿到旧列表,中途该键被delete后又被外部写回
save 是公开 API,异常会被 submitAsync 报到控制台,静默删除不会。建议要么保留「无值即抛错」,要么在 KDoc 里明确写出「若键不存在于缓存,将删除数据库中对应行」。目前 save 的注释仍是「保存指定键的值到数据库」,没提删除语义。
🟡 问题 4 — forcedSet(sync = true) 多出一次数据库写
// 旧
playerDataContainer[it]?.source?.set(key, value.toString()) // 仅改内存
// 新
playerDataContainer[uniqueId]?.set(key, stringValue) // 改内存 + 排一次异步写库DataContainer.set 会走 updateValue(..., updateSource = true) 并排程写库,而上一行 database[targetUser, key] = stringValue 已经写过一次。同一值写两遍,结果幂等,但对「穿透缓存」这个方法名来说多了一次往返,高频调用时是可观的额外负载。
另外这次异步排程可能在关服阶段抛 RejectedExecutionException,使原本只做同步写的 forcedSet 变得可能抛错。
顺带说明:这里对 UUID.fromString 加 runCatching 是真修复——旧代码 UUID.fromString(targetUser)?.let{} 的 ?. 毫无作用(该方法返回非空类型),非法字符串会直接抛 IllegalArgumentException。
如果目的只是同步缓存,建议给 DataContainer 留一个只改 source 不排程的内部方法。
🟡 问题 5 — checkUpdate 变成主线程上 O(全部键) 的同步遍历,且 writeStates 不回收
// 旧:只遍历有延迟任务的键,通常为空
updateMap.filterValues { it < now }.forEach { ... }
// 新:遍历该容器写过的所有键,每个键都进 synchronized
writeStates.forEach { (key, state) -> synchronized(state) { ... } }@Schedule(period = 20, async = false)(Schedule.kt 默认 async = false)意味着这段在主线程每秒执行一次。writeStates 只增不减,容器存活期间写过的每个键都会留一个条目。200 人在线 × 每人 50 键 = 每秒 10000 次 synchronized。
单次开销极小,但这是纯粹为了延迟写而付的固定成本,而绝大多数 tick 里没有任何 deadline 到期。建议:
- 维护一个单独的「有 deadline 的键」集合,
checkUpdate只遍历它(等价于旧updateMap的用途) - 或在
drainWrites成功且deadline == null && !ready时从writeStates移除条目
🟡 问题 6 — updateMap 作为公开字段,语义反转但仍对外暴露
val updateMap 是 public,旧语义是「now - delay,即恒已过期的时间戳」,新语义是「未来的 deadline」。外部插件若读它做判断(例如 updateMap[key] < now 判断是否待写),升级后结论完全相反。
现在 updateMap 已退化为 writeStates 的影子副本,仅用于兼容。建议标 @Deprecated 并说明新语义,或在兼容性说明中明确列出。
🟡 问题 7 — 跨节点迁移只有 JVM 内锁,removeDuplicateRows 失败不重试
migrationLock 是 ConcurrentHashMap<String, Any> 按 connectionUrl|table 分桶的进程内锁,多节点共享同一 MySQL 时无法互斥。两个节点同时启动会并发执行同一条大范围 DELETE ... WHERE id NOT IN (...),MySQL 下容易撞死锁或 lock wait timeout。
createUniqueIndex 有 MAX_INDEX_ATTEMPTS = 4 的重试,但 removeDuplicateRows(migrateWithRetry 里的 inTransaction { removeDuplicateRows(connection) })不在任何 try 内——一次死锁就直接抛出,插件加载失败。
PR 描述提到「两个服务器节点同时写入」的场景,但迁移阶段的跨节点保护实际不存在。建议:给 removeDuplicateRows 也套上重试,或用 SELECT ... FOR UPDATE / 建一张迁移标记表做跨节点互斥。
🔵 次要
a. 大表首次迁移会静默卡住主线程。 Database.init 在插件 enable 阶段同步执行,百万行表上的 GROUP BY user, key + DELETE ... NOT IN 可能耗时数分钟。目前只在删完之后才 PrimitiveIO.warning 报告删了多少行(warnDuplicateRows),迁移前没有任何日志。用户看到的现象是「服务器启动卡死」。建议 countDuplicateRows > 0 时先 info 一行「正在迁移 N 行重复数据,请勿中断」。
b. 方言判断两套标准并存。 迁移路径用 connection.metaData.databaseProductName.contains("SQLite"),upsert 路径用 when (type) { is TypeSQL -> ...; is TypeSQLite -> ... }。自定义 Type 包 SQLite 主机时两套判断会分叉(走 upsertGeneric 但走 migrateSQLite)。统一到其中一种更稳。
c. upsertGeneric 在「值未变化」时会误抛。 逻辑是 updateValue > 0 才算成功;MySQL 下 UPDATE 把 value 写成与原值相同时 affected rows 返回 0,于是走到 insert → 撞唯一约束 → 再 updateValue 仍返回 0 → throw ex。仅影响自定义 Type(TypeSQL/TypeSQLite 都不走这条路),但建议把判定改为「约束冲突且 get(user, key) != null 即视为成功」。
d. SQLite 索引名是库级唯一,但 readIndices 只查本表。 resolveUniqueIndexName 基于本表已有索引挑名字,而 SQLite 的索引名在整个 db 文件内唯一。若名字被其他表的索引占用,CREATE UNIQUE INDEX IF NOT EXISTS 会静默什么都不做,随后 findUniqueKeyIndex 返回 null,报 Unable to create a unique player key index。名字含表名与 hash,碰撞概率极低,但 IF NOT EXISTS 在这里掩盖了真实错误——去掉它反而能拿到清晰的 index already exists 报错。
e. INSERT OR REPLACE 会重置未列出的列。 SQLite 的 OR REPLACE 是删旧行再插新行。若用户手工给表加过额外列,那些列会被重置为默认值/NULL。TypeSQLite 只定义 user/key/value,属边缘情况,可在兼容性说明里提一句。
f. NOTNULL 只对新建表生效。 createTable 走 CREATE TABLE IF NOT EXISTS,已存在的表不会 ALTER。所以「数据表缺少可靠约束」在老库上只补了唯一索引,NOT NULL 仍缺失。这是安全的选择(不动老库结构),但 PR 描述读起来像是约束已全面建立,建议澄清。
g. junit 版本与根 build 不一致。 根 build.gradle.kts:44-45 固定 5.8.1,本 PR 引入 junit-jupiter:5.10.2。Gradle 会把 API 提升到 5.10.2,而 testRuntimeOnly 的 engine 仍写死 5.8.1,靠传递依赖补齐。能跑,但版本管理上不干净,建议与根一致或统一升级。
🟢 已核对无误
| 项 | 结论 |
|---|---|
(user, key) 复合唯一约束 |
旧 set() 的 if (get(...) == null) insert else update 确有先查后插竞态,修复方向正确 |
MySQL onDuplicateKeyUpdate |
ActionInsert.kt:64 存在该 API,DuplicateUpdateBehavior.update 走参数化,无注入 |
NOTNULL + KEY 是否生成非法 SQL |
否。ColumnSQL.kt:80 把 KEY/UNIQUE_KEY 从行内定义中过滤掉,只留 NOT NULL,索引单独成句 |
Table.update / insert 返回值 |
Table.kt:54,62 均返回 Int 影响行数,upsertGeneric 的 > 0 判断成立 |
setupQuoterForHost 线程安全 |
Util.kt:22 是 ThreadLocal,异步线程各自独立,无跨线程竞争 |
| 特殊表名引用 | asFormattedColumnName(Util.kt:76-91)按当前 quoter 加引号,player-data 正确包成反引号;测试已覆盖 |
db.table 形式表名 |
readIndices 用 linkedSetOf(table.name, table.name.substringAfterLast('.')) 兼顾两种,考虑周到 |
SQLite 用 rowid / SQL 用 id |
TypeSQLite 无 id(),TypeSQL 有(ColumnBuilder.kt:42),分支正确 |
| 归并保留最新行 | MAX(rowid) / MAX(id) 保留最大主键,单行分组也会被保留(MAX 即自身),不会误删 |
| 按列定义校验索引 | findUniqueKeyIndex 比对 columns.values == ["user","key"] 且 !nonUnique,不再只看名字;same named index on wrong columns 测试覆盖到位 |
revision/ready/running 状态机 |
旧值回写被 revision 比对拦住;startIfNeeded 保证同一键只有一个 drain 在跑,无重复排程 |
| 写库 I/O 在锁外 | drainWrites 只在取快照/判定续跑时持锁,database[...] 调用在 synchronized 之外,不会长期持锁 |
排程失败回滚 running |
scheduleWrite catch 中复位 running = false,避免永久卡在「已安排未执行」 |
| 写失败后有新值则重排 | drainWrites catch 内判 revision != snapshot.revision && ready,重排并 addSuppressed,异常不被吞 |
deadlineAfter 溢出保护 |
显式判 now > Long.MAX_VALUE - delay,避免负 deadline 导致立即落库 |
inTransaction 恢复 autoCommit |
finally 中 runCatching { connection.autoCommit = originalAutoCommit },池化连接归还前状态干净 |
UUID.fromString 加 runCatching |
真修复。旧 ?.let 对非空返回类型无意义,非法字符串会抛 IllegalArgumentException |
| Hikari 测试桩包名 | com.zaxxer.hikari_4_0_3 对应 database 模块 shadow 重定位后的包名,注释解释清楚 |
测试可见 internal asyncExecutor |
Kotlin test sourceset 默认 associateWith main,可访问 internal,编译成立 |
总结
状态机与唯一约束这两块是这批 PR 里质量较高的实现,revision 比对、锁外 I/O、排程失败复位 running、按列校验索引,都处理得比较细。
需要合并前处理的两点:
- 问题 1(
setDelayed生效后缺 flush)——这是本 PR 新引入的数据丢失路径,与 PR 目标相反,建议补flush()+ 释放/DISABLE时调用。 - 问题 2(与 #709 冲突)——git 冲突必然发生,且机械合并会让
ensureUniqueKeyIndex()逃出 #709 的try块导致连接池泄漏。需要确定合并顺序。
问题 3-7 建议一并处理,其中问题 3(save 静默删除)和问题 6(updateMap 语义反转)至少要写进兼容性说明。
说明:本次审阅未实跑 gradle 测试(含 PR 描述中列出的构建命令),全部结论基于 patch 与仓库源码推导,已逐条注明依据位置。测试用例本身的设计与断言我逐个读过,覆盖面对得上 PR 描述。
原有问题
玩家数据库的表约束、首次写入和延迟缓存写回不是一个完整的原子流程:
(user, key)复合唯一约束,并发首次写入采用先查后插时可能同时创建多行。典型触发场景与后果
本 PR 修改
(user, key)复合唯一约束;升级时按稳定顺序保留最新记录,再创建正确索引。修改目的
保证同一玩家键在并发创建、连续更新和插件关闭场景下仍只有一个确定的最终值,避免重复记录、数据回档和迁移后约束失效。
兼容性与行为变化
验证
./gradlew :module:database:database-player:build :module:database:database-player-redis:build --rerun-tasks --no-parallel --stacktracegit diff --checkRefs #703