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.

Sign up for GitHub

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

Jump to bottom

Fixed the issue that the TCP connection was interrupted when setting a proxy #9708

Closed
wants to merge 3 commits into from
Closed

Fixed the issue that the TCP connection was interrupted when setting a proxy #9708

wants to merge 3 commits into from

Conversation

Zuoqiu-Yingyi
Copy link
Contributor

@Zuoqiu-Yingyi Zuoqiu-Yingyi commented Nov 21, 2023 •
edited

  • Please commit to the dev branch
  • For contributing new features, please supplement and improve the corresponding user guide documents
  • 在没有其他请求时同步调用 setProxy 函数, 避免在中断所有 TCP 连接时中断部分向内核发出的请求
    Call the setProxy function synchronously when there are no other requests to avoid interrupting some requests to the kernel when all TCP connections are interrupted

TESTED

REF: https://ld246.com/article/1700548414564

@github-actions github-actions bot changed the base branch from master to dev November 21, 2023 13:05
@github-actions GitHub Actions
Copy link

Your PR was set to target master, PRs should be target dev
The base branch of this PR has been automatically changed to dev, please check that there are no merge conflicts

Zuoqiu-Yingyi added a commit to siyuan-community/siyuan that referenced this pull request Nov 21, 2023
@Zuoqiu-Yingyi
fix(proxy): Fixed the issue that the TCP connection was interrupted w…
cb68e91 
…hen setting a proxy

siyuan-note/siyuan#9708
@Zuoqiu-Yingyi

This comment was marked as outdated.

Sign in to view
@Zuoqiu-Yingyi

This comment was marked as outdated.

Sign in to view
@Zuoqiu-Yingyi Zuoqiu-Yingyi force-pushed the fix/electron-set-proxy branch 2 times, most recently from 30b75a4 to 7452801 Compare November 21, 2023 19:30
Vanessa219
Vanessa219 reviewed Nov 22, 2023
View reviewed changes
app/src/util/serviceWorker.ts Outdated
if (!("serviceWorker" in navigator) || typeof (navigator.serviceWorker) === "undefined" ||
!("caches" in window) || !("fetch" in window)) {
return;
export const registerServiceWorker = async (scriptURL: string, scope = "/", workerType: WorkerType = "module") => {
Copy link
Member

Choose a reason for hiding this comment

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

和 proxy 没有关系的代码麻烦不要提交。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

和 proxy 没有关系的代码麻烦不要提交。

这里是为了避免加载 service-worker 时对现有请求造成不可预期的影响，是相关的

Copy link
Member

Choose a reason for hiding this comment

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

这个只有在浏览器端和移动端使用，和 electron 的 setProxy 无关。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个只有在浏览器端和移动端使用，和 electron 的 setProxy 无关。

我在使用中发现在 electron 中使用 <iframe> 或 <webview> 标签访问 /stage/build/desktop/ 与 /stage/build/mobile/ 也会触发 service-worker

Copy link
Member

Choose a reason for hiding this comment

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

registerServiceWorker 这个好像还是你 PR 的，是不是这些情况排查漏了。这个问题应该从源头上改进，而不是对错误进行兼容。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

registerServiceWorker 这个好像还是你 PR 的，是不是这些情况排查漏了。这个问题应该从源头上改进，而不是对错误进行兼容。

原有实现暂时也未出现问题, 这里的改进是为了消除不确定性

Copy link
Member

Choose a reason for hiding this comment

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

目前的 registerServiceWorker 为什么要在 electron 中运行？感觉是没有必要的。不是出现问题才是有 bug。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不是 registerServiceWorker 要在 electron 中运行, 而是无法完全阻止 registerServiceWorker 在 electron 中运行, 因为 <iframe> 与 <webview> 的上下文可能是完全隔离的

不过我刚刚有一个想法, 在 electron 环境中检测到 service-worker 后可以自动注销该 worker, 我先去测试一下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Vanessa219
提交了一个解决方案: 在启动时从桌面端与移动端 App 中注销当前存在的 service-worker

详情请参考 36bad6c

Copy link
Member

Choose a reason for hiding this comment

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

能不能不要搅和在这个 issue 里面呀 😭

@Vanessa219
Copy link
Member

目前有 2 个问题需要先确认一下：

  1. closeAllConnections 这个是不是必须的？
  2. 对于本地请求要如何跳过代理？

@Zuoqiu-Yingyi
Copy link
Contributor Author

  1. closeAllConnections 这个是不是必须的？

应该是需要的, 不然可能在遇到 TCP 长连接时会引入更加复杂的问题

  1. 对于本地请求要如何跳过代理？

electron 的 session.setProxy 参数可以使用 pacScript 指定 PAC 文件作为代理策略或者使用 proxyBypassRules 指定排除的地址
不过这里最好可以由用户自定义, 默认的配置可以设置为跳过本机地址
详情请参考 session | Electron

@Vanessa219
Copy link
Member

closeAllConnections 把长链接断开了，好像没有机制重新再次建立长链接？如果不断的话会有什么问题？

需要代理 kernel 请求的场景是？

@Zuoqiu-Yingyi
Copy link
Contributor Author

closeAllConnections 把长链接断开了，好像没有机制重新再次建立长链接？

用户发起请求时可以重新建立长连接

如果不断的话会有什么问题？

可能会出现切换代理后用户的请求复用切换代理前的长连接, 与用户切换代理的期望不符

需要代理 kernel 请求的场景是？

用户在如下场景中都有绕过代理的需求

  • 用户访问本机服务 (包括本机内核服务)
  • 用户访问局域网内其他设备的服务
  • 用户访问使用虚拟网卡桥接的虚拟机

@Vanessa219
Copy link
Member

Vanessa219 commented Nov 22, 2023 •
edited

代理主要用途为外网非本地的，无法保证外网的程序都有这个机制。不行的话，切换的时候就直接刷界面吧。

本机访问不太建议走代理，首先没有必要，其次会减慢速度。

/api/system/getConf 之前的请求都为本机的，代码基本可以不用动。 css 什么的还是应该优先进行加载。

@Zuoqiu-Yingyi
Copy link
Contributor Author

代理主要用途为外网非本地的，无法保证外网的程序都有这个机制。不行的话，切换的时候就直接刷界面吧。

electron 的代理都是 web 服务使用, HTTP 可以保证都有这个机制, 无需刷新界面

本机访问不太建议走代理，首先没有必要，其次会减慢速度。

这里可能理解有误, 之前提到的 绕过代理 都是指 不经代理直接访问

@Vanessa219
Copy link
Member

websocket 也可以？

没有误解，一开始我问的是，而不是绕过代理的需求

需要代理 kernel 请求的场景是？

@Zuoqiu-Yingyi
Copy link
Contributor Author

websocket 也可以？

我这里切换代理前后都是正常连接的

需要代理 kernel 请求的场景是？

这个场景不多
用于流量分析?

@Vanessa219
Copy link
Member

非常感谢你前期的调研和贡献。基于你前期的调研，今早和 D 研究测试了一下，现在的 PR 在某些场景下还是有问题，因此只能关闭这个 PR，非常抱歉。
为了程序的稳定和性能我们最终决定按照 @zuoez02 说的在 main.js 里面去 setProxy，设置中更新代理后对界面进行刷新。

@Vanessa219 Vanessa219 closed this Nov 23, 2023
@zuoez02
Copy link
Contributor

zuoez02 commented Nov 23, 2023

更新默认session的proxy，好像不刷新也能直接用吧？

@Vanessa219
Copy link
Member

长链接不会断开，使用不了更新后的。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vanessa219 Vanessa219 Awaiting requested review from Vanessa219

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Zuoqiu-Yingyi @Vanessa219 @zuoez02