chore: revert send data by chunk in websocket (#3988) #4477
Conversation
|
/next |
|
为什么 revert 了 |
|
🎉 PR Next publish successful! 3.8.3-next-1741917543.0 |
Walkthrough此次提交在多个模块中对功能逻辑及测试进行了调整和简化。TreeNode 模块中修改了 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FrameDecoder
participant DataEmitter
Client->>FrameDecoder: push(data)
loop While frame available
FrameDecoder->>FrameDecoder: readFrame()
alt 有有效帧
FrameDecoder->>DataEmitter: 数据事件触发
end
end
sequenceDiagram
participant Sender
participant ReconnectWS
participant Socket
Sender->>ReconnectWS: send(data)
ReconnectWS->>Socket: 直接调用 socket.send(data)
Suggested labels
Suggested reviewers
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. 🔧 ESLint
yarn install v1.22.22 Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
packages/components/src/recycle-tree/tree/TreeNode.ts (1)
41-45: 简化的 spliceArray 函数实现移除了条件检查：
if (deleteCount === 0 && (!items || items.length === 0)) { return arr; }，现在函数总是创建并返回新数组。这种变化使实现更加一致，但可能会导致在不需要修改数组时产生额外的内存分配和复制开销。
如果原条件检查是为了性能优化（避免不必要的数组复制），考虑添加注释说明为何选择移除此优化，特别是在处理大型数组或频繁调用的场景中：
export function spliceArray(arr: number[], start: number, deleteCount = 0, items?: number[] | null) { + // 为了简化实现和确保一致的返回类型，总是创建新数组 const a = arr.slice(0); a.splice(start, deleteCount, ...(items || [])); return a; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/components/src/recycle-tree/tree/TreeNode.ts(2 hunks)packages/connection/__test__/browser/index.test.ts(2 hunks)packages/connection/__test__/common/buffers.test.ts(0 hunks)packages/connection/__test__/common/frame-decoder.test.ts(4 hunks)packages/connection/__test__/node/index.test.ts(2 hunks)packages/connection/__test__/node/ws-channel.test.ts(3 hunks)packages/connection/src/common/buffers/buffers.ts(0 hunks)packages/connection/src/common/connection/drivers/frame-decoder.ts(6 hunks)packages/connection/src/common/connection/drivers/reconnecting-websocket.ts(3 hunks)packages/connection/src/common/connection/drivers/stream.ts(1 hunks)packages/connection/src/common/connection/drivers/ws-websocket.ts(1 hunks)packages/connection/src/common/constants.ts(0 hunks)packages/connection/src/common/fury-extends/one-of.ts(0 hunks)packages/connection/src/node/common-channel-handler.ts(1 hunks)packages/core-browser/__tests__/bootstrap/connection.test.ts(2 hunks)
💤 Files with no reviewable changes (4)
- packages/connection/src/common/constants.ts
- packages/connection/src/common/fury-extends/one-of.ts
- packages/connection/test/common/buffers.test.ts
- packages/connection/src/common/buffers/buffers.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: 🚀🚀🚀 Next Version for pull request
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: build-windows
🔇 Additional comments (30)
packages/core-browser/__tests__/bootstrap/connection.test.ts (2)
3-3: 移除了不必要的导入从
@opensumi/ide-core-common中移除了sleep导入，这与后续代码中使用 Promise 和 setTimeout 代替 sleep 函数的变更相匹配。
37-41: 使用原生 Promise 替代 sleep 函数将原先使用的
sleep(4000).then(...)替换为手动创建的 Promise 和 setTimeout，功能保持一致，但移除了对 sleep 工具函数的依赖。packages/connection/src/node/common-channel-handler.ts (1)
45-46: 改进代码可读性将之前单行操作拆分为两行，先创建
wsConnection变量，再传递给receiveConnection方法。这种写法增强了代码可读性和可调试性，不影响功能。packages/connection/__test__/browser/index.test.ts (3)
1-1: 调整导入语句直接从
@opensumi/ide-connection导入furySerializer，简化了导入路径。
24-43: 简化 WebSocket 连接处理逻辑移除了
WSWebSocketConnection包装层，直接使用 socket 对象进行消息处理。这种变更简化了代码结构，移除了一层抽象，使消息处理更加直接。
27-33: 直接使用 socket 发送消息使用
socket.send()替代之前可能使用的connection.send()，与移除 WebSocket 连接包装层的变更保持一致。packages/connection/__test__/node/ws-channel.test.ts (3)
88-88: 简化测试消息内容将之前可能发送不同消息（'hello' + i）的方式简化为每次发送相同消息（'hello'）。这种变更简化了测试逻辑，不影响测试目的。
98-98: 减少测试超时时间将测试超时时间从 30 秒减少到 20 秒，可能是因为简化后的代码执行更快，不需要那么长的等待时间。
152-152: 减少测试超时时间与前一个测试一样，将超时时间从 30 秒减少到 20 秒，保持测试配置的一致性。
packages/connection/__test__/node/index.test.ts (2)
11-11: 导入语句的修改反映了实现的简化移除了
ChannelMessage的导入，与代码中不再使用该类型相符。这是符合代码简化趋势的积极变化。
68-75: 消息处理逻辑简化，直接使用 WebSocket 连接原代码使用了
wrappedConnection（通过wrapSerializer(wsConnection, furySerializer)创建）来处理消息。新实现直接使用原始 WebSocket 连接并手动反序列化消息。这种变化使代码更加直接，减少了抽象层级，但需要手动处理序列化/反序列化过程。
本次重构遵循了 PR 标题"撤销在 websocket 中分块发送数据"的目标，简化了连接处理逻辑。
packages/components/src/recycle-tree/tree/TreeNode.ts (1)
572-577: 更新了注释中的引号格式修改了注释中的标准引号为排版引号，使文档样式更加一致。这是一个正面的改进。
packages/connection/__test__/common/frame-decoder.test.ts (6)
38-38: 移除 .dump() 方法调用从
LengthFieldBasedFrameDecoder.construct(v)中移除了.dump()调用，这与其他文件中 frame-decoder 相关修改保持一致。这一修改符合 PR 目标，简化了数据处理流程。
51-52: 简化了数据包构造过程移除了创建混合数据包时的
.dump()调用，使实现更加一致。
62-63: 测试用例中简化了帧创建在测试用例中同样移除了
.dump()调用，与模块实现的变化保持一致。
72-89: 改进了测试逻辑和内存使用监控测试实现做了几项改进：
- 添加了
done回调以正确完成异步测试- 简化了断言逻辑，使用直接的等式检查
- 添加了内存使用日志记录
- 改进了分块处理逻辑，使用循环按
pressure值定义的大小来推送数据块这些变化使测试更加健壮和透明，能够更好地监控大数据包处理时的内存使用情况。
改进后的测试实现更加清晰，并提供了有价值的性能监控数据。
92-115: 简化了多帧解码测试实现此测试用例通过以下方式得到了简化：
- 使用计数器代替复杂的断言逻辑
- 添加了正确的异步测试完成处理
- 加入了内存使用监控
- 使用分块写入来更真实地模拟流数据处理
这些改进使测试更加准确地模拟了实际使用场景，特别是处理大数据包时的行为。
117-133: 改进了无效长度信息的流解码测试此测试用例模拟了更真实的场景，通过使用很小的块大小(pressure = 2)来测试当头部和负载被分离时的解码情况。
这种测试方法可以更好地验证解码器在处理碎片化数据流时的健壮性。
packages/connection/src/common/connection/drivers/ws-websocket.ts (2)
11-12: 简化了 send 方法的实现将之前复杂的基于队列的实现简化为直接调用 socket 的 send 方法。这样的改动移除了消息队列管理的复杂性，使实现更加直接和简洁。
需要注意的是，这种改动假设底层的 WebSocket 实现能够自行处理可能的背压问题。请确认你的应用场景中不需要队列机制来管理大量并发消息。
15-21: 简化了消息接收逻辑移除了之前可能存在的解码器，直接使用 WebSocket 的 'message' 事件来接收消息。这种改动与 send 方法的简化保持一致，整体上降低了代码的复杂度。
改动后的实现更加直接，易于理解和维护。
packages/connection/src/common/connection/drivers/frame-decoder.ts (6)
3-3: 采用 Emitter 模式替代原有的监听器模式使用
Emitter类来处理数据事件，相比之前的监听器实现，这提供了更加统一和可控的事件处理机制。这种变更使代码结构更加清晰，并且在事件处理上更具扩展性。Also applies to: 17-18
27-34: 提高了长度字段注释的可读性并添加了类属性将原本的私有常量改为类属性
lengthFieldLength，使其更容易被引用和扩展。同时完善了注释内容，使其更加清晰易懂。这有助于开发者理解该属性的用途和约束。
42-48: 简化了数据处理循环将原来的异步数据处理改为同步循环处理，这样简化了代码的复杂性，同时也提高了执行效率。这种改进符合"撤销通过块发送数据"的整体目标。
51-75: 重构 readFrame 方法提高了代码清晰度重构后的
readFrame方法逻辑更为清晰，返回布尔值来指示是否完成帧的读取，并直接使用dataEmitter.fire()来发送数据。这种改进使方法的职责边界更加明确，提高了代码的可维护性。
157-157: 更新了资源释放逻辑更新了
dispose方法，确保dataEmitter正确释放，这对于防止内存泄漏很重要。这是良好的资源管理实践。
161-168: 优化了静态 construct 方法添加了静态
writer属性，并简化了construct方法，避免了每次调用时创建新的 writer 实例的开销。这种改进在需要频繁调用该方法的场景下可以提高性能。packages/connection/src/common/connection/drivers/reconnecting-websocket.ts (4)
34-54: 重构了 onMessage 方法以处理不同类型的消息完全重写了
onMessage方法，新的实现处理了多种可能的消息类型（Blob、ArrayBuffer或Node.js Buffer），然后将其转换为 Uint8Array。这种改进增强了代码的健壮性，使其能够在不同环境中正常工作。新的实现也使用 Promise 来处理异步数据转换，这在处理 Blob 类型时特别有用。
16-17:❓ Verification inconclusive
简化了 send 方法
与 WSWebSocketConnection 类似，这里将 send 方法从返回 Promise 简化为直接调用 socket.send 并返回 void。这种改动移除了之前可能存在的队列机制和异步处理，使实现更加直接。
请确认此变更对于客户端代码的影响，特别是那些依赖返回 Promise 的代码可能需要相应调整。
🏁 Script executed:
#!/bin/bash # 检查是否有代码依赖 send 方法返回的 Promise rg "\.send\(.*\)\.then" --glob "*.ts"Length of output: 39
请手动再确认客户端代码中是否存在依赖
send()返回 Promise 的情况经过初步 grep 检查，并未发现有代码使用
.send(…)后直接调用.then的情况。此次修改将send方法从返回Promise<void>调整为直接调用socket.send并返回void，类似于WSWebSocketConnection类中的实现。这一改动简化了实现，移除了之前可能存在的队列机制和异步处理。建议开发者执行以下操作：
- 请手动核查客户端代码是否有依赖
send方法返回 Promise 的使用，例如利用await或.then处理返回结果的场景。- 如果确认所有使用场景均不依赖异步返回，则此次修改是安全的。
12-12:✅ Verification successful
移除了 constructor 的 protected 访问修饰符
将构造函数从
protected constructor改为普通的constructor，可能是为了允许直接实例化这个类。请确认这是有意为之的改变，以及是否会影响到依赖此访问级别的代码。
🏁 Script executed:
#!/bin/bash # 检查是否有其他地方依赖这个类的受保护构造函数 rg -A 2 -B 2 "ReconnectingWebSocketConnection" --glob "!reconnecting-websocket.ts"Length of output: 3169
关于构造函数访问修饰符修改的核查结果
经过对代码库的搜索，确认目前对
ReconnectingWebSocketConnection的引用均通过静态方法（例如forURL）进行实例化，没有发现任何地方直接使用构造函数进行实例化。因此，将构造函数从protected改为普通的constructor不会对现有依赖该访问级别的代码产生负面影响。不过，请留意：
- 确保文档和代码注释中说明：该改动是有意为之，有助于允许直接实例化，但也可能引入直接构造对象的风险。
- 代码规范性：如果需要强制使用工厂方法以确保实例化逻辑统一，建议在代码审查和未来演进时再次评估这一设计决策。
95-95:❓ Verification inconclusive
简化了 dispose 方法
将 dispose 方法简化为不执行任何操作。这可能是因为类中的资源（如 socket）不需要在此处显式释放，或者释放责任已转移到其他地方。
请确认这不会导致资源泄漏，特别是在组件生命周期结束时是否有其他方式确保资源正确释放。
🏁 Script executed:
#!/bin/bash # 检查此类的使用方式，特别是dispose方法的调用情况 rg "\.dispose\(\)" --glob "*.ts" | rg -C 3 "ReconnectingWebSocketConnection"Length of output: 76
请确保资源正确释放
目前通过代码库搜索未发现对
ReconnectingWebSocketConnection.dispose()方法的显式调用，但由于现有验证结果不够充分，请手动确认该类在组件生命周期结束时是否有其他机制释放诸如 socket 等资源，以防止产生资源泄漏问题。
| send(data: Uint8Array): void { | ||
| const handle = LengthFieldBasedFrameDecoder.construct(data).dumpAndOwn(); | ||
| try { | ||
| this.writable.write(handle.get(), (error) => { | ||
| if (error) { | ||
| console.error('Failed to write data:', error); | ||
| } | ||
| }); | ||
| } finally { | ||
| handle.dispose(); | ||
| } | ||
| const result = LengthFieldBasedFrameDecoder.construct(data); | ||
| this.writable.write(result, () => { | ||
| // TODO: logger error | ||
| }); |
There was a problem hiding this comment.
简化的 send 方法实现更加直接
代码移除了中间缓冲区的管理，直接使用 LengthFieldBasedFrameDecoder.construct(data) 的结果写入流。原先对错误处理的实现被简化为一个 TODO 注释。
这种实现更加简洁，但需要注意以下几点：
- 错误处理现在缺失，只有一个 TODO 注释
- 去掉了
dumpAndOwn()及其相关的资源管理
请确保在发布前增加适当的错误处理逻辑，以避免在写入失败时出现静默错误。
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4477 +/- ##
==========================================
- Coverage 53.24% 53.19% -0.06%
==========================================
Files 1663 1663
Lines 102619 102488 -131
Branches 22201 22187 -14
==========================================
- Hits 54639 54517 -122
+ Misses 39920 39909 -11
- Partials 8060 8062 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Types
Background or solution
Changelog
Summary by CodeRabbit