Skip to content
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

feat: rpcProtocol add timeout control #2587

Merged
merged 5 commits into from
Apr 18, 2023
Merged

feat: rpcProtocol add timeout control #2587

merged 5 commits into from
Apr 18, 2023

Conversation

pipiiiiii
Copy link
Contributor

@pipiiiiii pipiiiiii commented Apr 14, 2023

Types

  • 🎉 New Features

Background or solution

🤖 Generated by Copilot at 1a6b4dd

  • Add an optional timeout property to the RPC protocol interface (link)
  • Define a default timeout value for the RPC protocol (link)
    • Declare and initialize a private map of timeout handles for each call ID (link, link)
    • Set and clear timeout callbacks for each call and reply using the protocol or default timeout value (link, link)
    • Define a private method to handle timeout scenarios and reject pending replies with an error (link)

Issue: #2547

Changelog

  • feat: rpc message add timeout handle, default 30s

🤖 Generated by Copilot at 1a6b4dd

Add timeout support to RPC protocol in rpcProtocol.ts. This enables rejecting RPC calls that take too long to respond and avoiding potential resource leaks.

@pipiiiiii pipiiiiii requested review from erha19 and Ricbet April 14, 2023 06:16
@opensumi opensumi bot added the 🎨 feature feature required label Apr 14, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2023

ChatGPT Code Review:

CodeReview:

  1. 这个 diff 文件新增了一些代码,主要实现了 RPCProtocol 的超时控制功能。
  2. 新增了一个 timeout 属性,可以通过设置 timeout 来控制 RPC 请求的超时时间。
  3. 通过 setTimeout 来实现 RPC 超时,一旦超时就会 reject 请求,并删除掉 RPC 的回调函数和定时器句柄。
  4. 由于之前没有超时控制,可能存在一些请求过程中一直等待回调,导致阻塞进程的情况出现。这个变更可以有效避免这种情况的出现。
  5. 变量命名规范,代码风格都符合规范。

建议:

  1. 如果要使用这个超时控制的功能,建议在 RPC 请求之前就设置好超时时间。
  2. 如果超时控制在一些测试用例中没有必要,可以将 timeout 置为 undefined 以关闭超时控制。
  3. 在函数 receiveOneMessage 中,建议先判断是否存在回调和定时器句柄,再进行操作。
  4. 变量命名规范、代码风格都符合规范,没有说明上文的错误。将 CodeReview 通过。

生成的 commit message:

feat(connection): add timeout control in RPCProtocol

新增 RPCProtocol 的超时控制功能,通过设置 timeout 属性来控制 RPC请求的超时时间。超时时间到达后,会reject请求的回调函数,并清除回调函数和定时器句柄,有效避免了请求过程中一直等待回调,导致阻塞进程的情况出现。

@Ricbet
Copy link
Member

Ricbet commented Apr 14, 2023

/publish

@github-actions
Copy link
Contributor

🎉 PR Next version 2.23.4-next-1681455781.0 publish successful! You can install prerelease version via npm install package@2.23.4-next-1681455781.0 @Ricbet

2.23.4-next-1681455781.0

/home/runner/work/_temp/_runner_file_commands/step_summary_9c44528b-7a15-4657-850a-d8f297e57564

Copy link
Member

@erha19 erha19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需求的背景感觉可以补充一下

packages/connection/src/common/rpcProtocol.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch coverage: 94.11% and project coverage change: +0.01 🎉

Comparison is base (fe9128d) 57.62% compared to head (fb76c14) 57.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2587      +/-   ##
==========================================
+ Coverage   57.62%   57.64%   +0.01%     
==========================================
  Files        1326     1327       +1     
  Lines       83559    83615      +56     
  Branches    17402    17417      +15     
==========================================
+ Hits        48154    48202      +48     
- Misses      32166    32175       +9     
+ Partials     3239     3238       -1     
Flag Coverage Δ
jsdom 52.65% <52.94%> (+0.01%) ⬆️
node 16.78% <94.11%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/connection/src/common/rpcProtocol.ts 85.07% <94.11%> (+1.37%) ⬆️

... and 24 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pipiiiiii
Copy link
Contributor Author

这个改动是之前修复 #2530 这个问题时发现的,我们现在通过 getProxy 发送消息之后,如果碰到没有消息接收方,并且使用了 await 这种写法,就会导致代码一直阻塞住,通常 http 请求都会有超时设置,像这种需要获取返回值的场景,增加一个默认的超时时间比较好。
@erha19 补充了测试用例,就是会出现这种异常的场景

Copy link
Member

@erha19 erha19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erha19 erha19 merged commit 33b63b0 into main Apr 18, 2023
11 checks passed
@erha19 erha19 deleted the feat/rpc-timeout branch April 18, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 feature feature required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants