Conversation
| sendMessage(data: TMessage) { | ||
| this.con.postMessage(data); | ||
| if (!this.con) { | ||
| console.warn("Attempted to sendMessage on a disconnected port."); |
There was a problem hiding this comment.
我怕现在的东西有写错的话,抛出错误会影响
如果只是 console.warn console.error 的话,你还是可以见到但不会直接影响现时的代码
(主要是 sendMessage 这个吧)
所以所有 if (!this.con) { 都 抛出错误?
There was a problem hiding this comment.
我认为抛出错误是因为有意外的情况,抛出来可以看到更详细的上下文内容
如果直接忽略,本身就已经影响了业务逻辑,继续执行后面的逻辑反而会出现更多的意外情况
这种 return; 的可以抛出错误,其它的打印日志 ,继续也行(没有影响原来的逻辑)
There was a problem hiding this comment.
你的 抛出错误 意思是 console.warn 改 console.error 然后 return; ?
还是要 throw new Error
There was a problem hiding this comment.
throw new Error
return; 替换为 throw new Error
There was a problem hiding this comment.
Pull request overview
这个 PR 旨在优化 MessageConnect 的内存管理和 Cleanup 机制,以防止潜在的内存泄漏。然而,经过详细审查,我发现这个 PR 引入了比原有实现更严重的问题,并且存在多个关键性的 bug 和设计缺陷。
Changes:
- 引入全局
listenerMgrEventEmitter 单例来管理所有连接的监听器 - 为每个 MessageConnect 实例添加唯一的
listenerId用于事件命名空间隔离 - 修改
onDisconnect接口签名以传递isSelfDisconnected参数 - 重构 cleanup 逻辑,在 disconnect 时显式触发 cleanup 事件
- 将
forwardMessage的 handler 改为 async 函数
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/message/extension_message.ts | 重构 ExtensionMessageConnect 类,引入全局 listenerMgr 和新的 cleanup 机制;删除了 sendMessage 错误处理中的 return 语句 |
| packages/message/window_message.ts | 重构 WindowMessageConnect 类,采用与 extension_message.ts 相同的全局 listenerMgr 模式 |
| packages/message/types.ts | 修改 MessageConnect.onDisconnect 接口签名,添加 isSelfDisconnected 参数(破坏性变更) |
| packages/message/mock_message.ts | 实现 MockMessageConnect 的 disconnect 和 onDisconnect 方法,传递 isSelfDisconnected 参数 |
| packages/message/server.ts | 将 forwardMessage 的 handler 改为 async,显式返回 undefined |
| disconnect() { | ||
| this.con.disconnect(); | ||
| if (!this.con) { | ||
| console.warn("Attempted to disconnect on a disconnected port."); | ||
| return; | ||
| } | ||
| this.isSelfDisconnected = true; | ||
| this.con?.disconnect(); | ||
| // Note: .disconnect() will NOT automatically trigger the 'cleanup' listener | ||
| listenerMgr.emit(`cleanup:${this.listenerId}`); |
There was a problem hiding this comment.
这里存在竞态条件问题。在 line 196 设置 isSelfDisconnected = true 后,line 197 调用 this.con?.disconnect(),然后 line 199 立即触发 cleanup。但是:
- Chrome 的
port.disconnect()会异步触发onDisconnect监听器(line 172 注册的 cleanup 函数) - Line 199 的
listenerMgr.emit('cleanup:...')会立即同步执行 - 这可能导致 cleanup 被执行两次(一次同步,一次异步),虽然有
if (this.con)保护,但这种设计容易产生混乱
建议:
- 要么只依赖 Chrome 的
onDisconnect自动清理(删除 line 199) - 要么在手动触发 cleanup 前先移除 Chrome 的监听器,避免重复清理
There was a problem hiding this comment.
bdc4a9e 推前了
eventemitter3 是同步的
假如有这个情况发生 (实际上不会因为 Chrome.runtime.Port 自己呼叫时不会触发 onDisconnected )
this.con 会先清空
保证一次
| if (!this.con) { | ||
| console.error("onDisconnect Invalid Port"); | ||
| } | ||
| listenerMgr.once(`onDisconnect:${this.listenerId}`, callback); |
There was a problem hiding this comment.
使用 listenerMgr.once 限制了每个 listenerId 只能有一个 onDisconnect 回调。但在 forwardMessage 函数(server.ts:282-283)中,对同一个连接调用了两次 onDisconnect:
fromConnect.onDisconnect(toCon.disconnect.bind(toCon));
toCon.onDisconnect(fromConnect.disconnect.bind(fromConnect));这意味着第二次调用会覆盖第一次的回调,导致其中一个不会被执行。这是一个严重的 bug,会破坏连接转发的清理逻辑。
There was a problem hiding this comment.
不会. EventEmitter3 的 once 不会 覆盖原有的callback
| onMessage(callback: (data: TMessage) => void) { | ||
| this.con.onMessage.addListener(callback); | ||
| if (!this.con) { | ||
| console.error("onMessage Invalid Port"); | ||
| } | ||
| listenerMgr.addListener(`onMessage:${this.listenerId}`, callback); |
There was a problem hiding this comment.
这里存在逻辑错误。当 this.con 为 null 时记录了错误日志,但仍然继续执行 listenerMgr.addListener,这会导致后续无法接收到任何消息(因为没有 Chrome port 在监听)。
建议改为:
- 如果
this.con为 null,应该直接返回或抛出错误 - 或者在添加监听器之前检查连接状态
| onDisconnect(callback: (isSelfDisconnected: boolean) => void) { | ||
| if (!this.target) { | ||
| console.error("onDisconnect Invalid Target"); | ||
| } | ||
| listenerMgr.once(`onDisconnect:${this.listenerId}`, callback); |
There was a problem hiding this comment.
与 extension_message.ts 相同的问题:
- 竞态条件:postMessage 和 cleanup emit 可能导致重复清理
- 使用
listenerMgr.once限制只能有一个 onDisconnect 回调,破坏了多次调用的能力 - 当 target 为 null 时继续执行
| onDisconnect(callback: (isSelfDisconnected: boolean) => void) { | ||
| if (!this.con) { | ||
| console.error("onDisconnect Invalid Port"); | ||
| } | ||
| listenerMgr.once(`onDisconnect:${this.listenerId}`, callback); |
There was a problem hiding this comment.
同样的错误:当 this.con 为 null 时记录错误但继续执行。这里应该在连接已断开的情况下直接返回或抛出错误,而不是继续注册监听器。
另外,listenerMgr.once 意味着这个回调只会被触发一次,但如果用户多次调用 onDisconnect 添加多个回调,只有最后一个会被执行(因为都注册到同一个事件键 onDisconnect:${this.listenerId} 上,且都使用 once)。这与原有的行为不一致。
原有实现允许添加多个 onDisconnect 监听器(通过 con.onDisconnect.addListener),现在的实现只支持一个。
| onMessage(callback: (data: TMessage) => void) { | ||
| this.EE.addListener(`connectMessage:${this.messageId}`, callback); | ||
| if (!this.target) { | ||
| console.error("onMessage Invalid Target"); | ||
| } | ||
| listenerMgr.addListener(`onMessage:${this.listenerId}`, callback); |
There was a problem hiding this comment.
与 extension_message.ts 相同的问题:当 this.target 为 null 时记录错误但继续执行。这会导致监听器被添加但永远不会被触发,造成无声的失败。建议在连接已断开时直接返回或抛出错误。
| toCon.onMessage(fromConnect.sendMessage.bind(fromConnect)); | ||
| fromConnect.onDisconnect(toCon.disconnect.bind(toCon)); | ||
| toCon.onDisconnect(fromConnect.disconnect.bind(fromConnect)); | ||
| return undefined; |
There was a problem hiding this comment.
返回 undefined 是明确的,但在原有代码中,这个分支没有显式返回值。虽然 JavaScript 中函数默认返回 undefined,但显式返回 undefined 可能会影响调用方的类型推断。
建议检查调用方是否期望这个返回值,如果不需要,可以省略 return 语句。
| return undefined; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
PR 标题
优化 MessageConnect 的内存管理及 Cleanup 机制
PR 描述
背景
在 ScriptCat 项目中,
MessageConnect是用于处理浏览器扩展、窗口消息传递和模拟环境下的长连接通信的核心接口。该接口涉及事件监听器(如onMessage和onDisconnect)的管理,以及与 Chrome 运行时端口或 PostMessage 的交互。在原有实现中,存在潜在的内存泄漏风险:当连接断开时,事件监听器可能未被完全移除,导致引用残留。这个 PR 旨在针对这些问题进行优化,基于对代码的审查和潜在问题分析(如监听器堆积引起的内存占用增加),以提升系统的稳定性和可维护性。项目可能在实际使用中遇到连接频繁建立/断开的场景(如脚本通信),优化后能减少资源浪费。
动机
主要修改内容
这个 PR 修改了 5 个文件,总共涉及约 100 行代码的调整(主要是重构和添加实现,无重大功能变更)。所有变化集中在
packages/message/目录下,聚焦于MessageConnect接口的实现和支持类。以下是逐文件分析:extension_message.ts(修改)
ExtensionMessageConnect类的cleanup函数,确保在端口断开时移除所有监听器(onMessage、onDisconnect、cleanup),并将con端口引用置为 null。同时,在disconnect()方法中显式触发cleanup事件(因为 Chrome 的.disconnect()不会自动调用监听器)。mock_message.ts(修改)
MockMessageConnect类实现了disconnect()和onDisconnect()方法,使用内部EventEmitter模拟断开行为(发出 "disconnect" 事件,并传递isSelfDisconnected = true)。MessageConnect的断开逻辑,而不依赖真实浏览器环境。server.ts(修改)
handler函数中,调整了连接桥接逻辑(fromConnect和toCon的消息/断开互绑),但实际 diff 显示无功能性变化。types.ts(修改)
MessageConnect接口。window_message.ts(修改)
WindowMessageConnect类的constructor和cleanup函数,确保移除模拟监听器(connectMessage和disconnect事件);在disconnect()中发送 "disconnect" 消息并触发cleanup事件,将target置为 null。如何整体改善项目