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

fix: namespace conflicts between extHost and extBrowser #2415

Merged
merged 2 commits into from
Mar 17, 2023

Conversation

yantze
Copy link
Member

@yantze yantze commented Mar 15, 2023

Types

  • 🐛 Bug Fixes

Background or solution

一般情况下,插件进程的日志会写入到 logDir/20230315/extHost.log 文件中,但由于 OpenSumi 在插件实现上有自己的 browser 层,会导致情况特殊,browser 写入的日志是通过 Node 进程写入的。

插件中 browser 层的日志是通过 Node 进程也同时写入到 extHost.log 中,在两个进程同时打开文件句柄,会导致 Windows 环境下的写锁保护机制报错,导致日志写入失败。

解决方法:插件的 Browser 使用另外一个命名空间,和插件进程区分开,保证不同的集成不会写入同一个文件,同时也能保证逻辑清晰。

closed #2383

Changelog

修复插件浏览器层的日志和插件进程的命名空间冲突

@github-actions github-actions bot added the 🐞 bug Something isn't working label Mar 15, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2023

ChatGPT Code Review:

对代码进行 CodeReview 可以从以下几个方面入手:

  1. 代码规范:有没有按照公司或者团队规定的编码规范编写代码;
  2. 代码结构:代码是否有冗余、重复或者不必要的部分;是否要进行抽象或者重构;
  3. 可读性:代码是否易读、易懂,是否有注释,是否有语义清晰的命名;
  4. 性能和安全性:代码边界是否得到了正确的检查和处理;代码执行的性能、资源、内存使用情况如何。

对于这份代码,比较明显的问题是:

  1. 文件名冲突:两个文件(debug/extension-debug-service.ts 和 api/main.thread.log.ts)都定义了 ILogger 变量,容易引起命名冲突;
  2. 代码注释:代码中缺乏注释,虽然代码量不是很大,但是加上一些注释可以方便其他人理解。

参考下面的 commit message 修改代码并提交:

feat(debug): use ILogger instead of ILogServiceClient

replace ILogServiceClient with ILogger in 3 files to avoid namespace conflicts

Signed-off-by: your name you@example.com

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (27135e4) 57.81% compared to head (1aba8e4) 57.82%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2415   +/-   ##
=======================================
  Coverage   57.81%   57.82%           
=======================================
  Files        1323     1322    -1     
  Lines       83315    83324    +9     
  Branches    17360    17361    +1     
=======================================
+ Hits        48165    48178   +13     
+ Misses      31939    31934    -5     
- Partials     3211     3212    +1     
Flag Coverage Δ
jsdom 52.84% <100.00%> (+<0.01%) ⬆️
node 16.71% <0.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...rowser/vscode/api/debug/extension-debug-service.ts 11.32% <100.00%> (-0.72%) ⬇️
...n/src/browser/vscode/api/main.thread.connection.ts 22.22% <100.00%> (-1.42%) ⬇️
...ension/src/browser/vscode/api/main.thread.debug.ts 65.85% <100.00%> (-0.21%) ⬇️
...xtension/src/browser/vscode/api/main.thread.log.ts 32.00% <100.00%> (-5.04%) ⬇️

... and 13 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.

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
Copy link
Member

erha19 commented Mar 17, 2023

/publish

@github-actions
Copy link
Contributor

🎉 PR Next version 2.22.9-next-1679025716.0 publish successful! You can install prerelease version via npm install package@2.22.9-next-1679025716.0 @erha19

2.22.9-next-1679025716.0

/home/runner/work/_temp/_runner_file_commands/step_summary_1cb4d9ce-17ea-4b38-8bf3-0fa3666b68c2

@erha19 erha19 merged commit 1025bba into main Mar 17, 2023
@erha19 erha19 deleted the fix/ext-browser-namespace branch March 17, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ide-electron 控制台出错
3 participants