-
-
Notifications
You must be signed in to change notification settings - Fork 27
refactor: optimize database server address tracing with WeakMap #1027
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
WeakMap 访问方式存在潜在 fatal 风险,且 Context key 语义仅记录首个连接的 server 信息
这里有两个点需要注意:
Error在 PHP 中,对
WeakMap使用$map[$obj]访问一个尚未存在的 key,会抛出Error: Object not contained in WeakMap,即使你用?? null包了一层也无法避免。只要出现“未经过createPdoConnection写入缓存、但走到了getPdo/getPdoForSelect分支”的情况(例如某些自定义连接器或未来扩展),就可能导致运行时 fatal。建议改为先通过
isset()/offsetExists()检查,再读取值,例如:这样即便某个 PDO 没有对应缓存,也只会静默跳过,不会中断请求。
Context::getOrSet(self::class, ...)使用self::class作为 key,意味着:getPdo/getPdoForSelect时会从 WeakMap 读取一份 server 信息并写入TRACE_DB_SERVER_*;如果你认为“一次请求只关心首个 DB server”是可接受的行为,那现在的实现是 OK 的;如果希望在读写分离或多库场景下精确标记每次查询的实际 server 信息,后续可以考虑:
getOrSet,在每次getPdo*时覆盖写入当前 PDO 对应的 host/port。第一点属于需要优先修复的稳定性问题,第二点则是行为语义上的设计权衡。
需要验证此评审意见中的两个关键点。让我检查代码上下文和PHP WeakMap行为。
🏁 Script executed:
Length of output: 54
🏁 Script executed:
Length of output: 1581
🏁 Script executed:
Length of output: 691
🏁 Script executed:
Length of output: 8885
🏁 Script executed:
Length of output: 6913
🏁 Script executed:
Length of output: 2096
🏁 Script executed:
Length of output: 810
🏁 Script executed:
Length of output: 1022
🏁 Script executed:
Length of output: 0
🏁 Script executed:
Length of output: 1982
🏁 Script executed:
Length of output: 919
🏁 Script executed:
Length of output: 0
🏁 Script executed:
Length of output: 0
🏁 Script executed:
Length of output: 404
🏁 Script executed:
Length of output: 0
🏁 Script executed:
Length of output: 123
🏁 Script executed:
Length of output: 247
🏁 Script executed:
Length of output: 447
🏁 Script executed:
Length of output: 2042
🏁 Script executed:
Length of output: 1218
🏁 Script executed:
Length of output: 691
🏁 Script executed:
Length of output: 1001
🏁 Script executed:
Length of output: 1982
🏁 Script executed:
Length of output: 3695
🏁 Script executed:
Length of output: 12697
🏁 Script executed:
Length of output: 52
🏁 Script executed:
Length of output: 40
🏁 Script executed:
Length of output: 156
🏁 Script executed:
Length of output: 1982
🏁 Script executed:
Length of output: 1068
🏁 Script executed:
Length of output: 412
🏁 Script executed:
Length of output: 112
🏁 Script executed:
Length of output: 2326
🏁 Script executed:
Length of output: 1005
🏁 Script executed:
Length of output: 52
🏁 Script executed:
Length of output: 2282
🏁 Script executed:
Length of output: 555
🏁 Script executed:
Length of output: 332
🏁 Script executed:
Length of output: 799
🏁 Script executed:
Length of output: 3339
🏁 Script executed:
Length of output: 208
🏁 Script executed:
Length of output: 1005
🏁 Script executed:
Length of output: 8
🏁 Script executed:
Length of output: 698
🏁 Script executed:
Length of output: 144
🏁 Script executed:
Length of output: 391
🏁 Script executed:
Length of output: 1078
🏁 Script executed:
Length of output: 1189
🏁 Script executed:
Length of output: 279
🏁 Script executed:
Length of output: 1246
🏁 Script executed:
Length of output: 0
🏁 Script executed:
Length of output: 164
🏁 Script executed:
Length of output: 1982
🏁 Script executed:
Length of output: 129
🌐 Web query:
💡 Result:
Short answer: WeakMap is an internal container class that implements ArrayAccess, so you must use object keys (not scalar/string keys) and its ArrayAccess rules are different from arrays — reading a missing key throws an Error, using a non-object key throws a TypeError, and you must call unset($wm[$obj]) or $wm->offsetUnset($obj) to remove an entry. See the RFC and the manual for the exact behaviours. [1][2]
Key points and examples:
References:
[1] PHP RFC: container/offset behaviour and WeakMap RFC notes.
[2] PHP manual: WeakMap class and its offsetGet/offsetSet/offsetUnset pages.
🌐 Web query:
💡 Result:
Short answer
References
🌐 Web query:
💡 Result:
Short answer — how Context::getOrSet behaves in Hyperf:
If you want links to the exact source file or an example showing passing a closure to getOrSet, tell me your Hyperf version and I’ll fetch the precise implementation lines.
Sources: Hyperf context package and docs; community write-up on RPC/context coroutine isolation; release notes mentioning getOrSet usage. [1][2][3]
🌐 Web query:
💡 Result:
Short answer: do not share a single PDO instance across coroutines. Open a connection per coroutine (or — better — borrow one from a connection pool and return it when done). Use Swoole/OpenSwoole's PDOPool or a Channel-based pool and store the borrowed PDO in the coroutine context so the same connection is reused within that request's coroutine and released via defer. This avoids race conditions and too-many-connections issues.
Why and how (concise):
Minimal pattern (pseudo-PHP):
References:
If you want, tell me which runtime you're using (Swoole, OpenSwoole, RoadRunner, Workerman, plain FPM) and I’ll give a concrete code example for that environment.
修复 WeakMap 访问的 Error 风险
第一个问题经验证属实且为关键问题。PHP WeakMap 对不存在的 key 直接访问会抛出
Error,即使使用?? null也无法避免,因为??操作符在短路评估前会先执行左侧的表达式。建议改用isset()检查:这样即便某个 PDO 对象未经
createPdoConnection缓存(例如通过自定义连接器创建),也只会静默跳过,不会中断请求。第二个问题关于
Context::getOrSet(self::class, ...)的语义是设计权衡而非缺陷——若当前实现"一次请求仅需记录首个数据库服务器信息"是可接受的需求,则现有实现 OK;若需支持读写分离或多库场景精确标记每次查询的实际服务器信息,可考虑后续使用更细粒度的 Context key。🤖 Prompt for AI Agents