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: ipc not work with worker_threads mode #5210

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Conversation

sjfkai
Copy link
Contributor

@sjfkai sjfkai commented Jun 13, 2023

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

lib/core/messenger

Description of change

Fix the issue where IPC does not work when startMode is set to 'worker_threads'.

This change depends on the modification made in "sendmessage #3". Please make sure that this change has taken effect before merging this one.

@fengmk2
Copy link
Member

fengmk2 commented Jun 13, 2023

@sjfkai 依赖可以升级到最新 2.0.0 版本 https://github.com/node-modules/sendmessage/actions/runs/5255496084

@sjfkai
Copy link
Contributor Author

sjfkai commented Jun 14, 2023

@sjfkai 依赖可以升级到最新 2.0.0 版本 https://github.com/node-modules/sendmessage/actions/runs/5255496084

改了,帮忙跑一下 test 看看?

Copy link
Member

@fengmk2 fengmk2 left a comment

Choose a reason for hiding this comment

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

+1

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (27a4942) 99.86% compared to head (0ae1687) 99.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5210   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files          36       36           
  Lines        3599     3603    +4     
  Branches      513      514    +1     
=======================================
+ Hits         3594     3598    +4     
  Misses          5        5           
Impacted Files Coverage Δ
lib/core/messenger/ipc.js 100.00% <100.00%> (ø)

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

@sjfkai
Copy link
Contributor Author

sjfkai commented Jun 14, 2023

看了一下 test 报错。应该跟本次修改关系不大。
node v20 下 app.httpRequest() 并发 4 个以上的时候会报错

@fengmk2 fengmk2 merged commit 03c8cf7 into eggjs:master Jun 15, 2023
16 of 18 checks passed
fengmk2 pushed a commit that referenced this pull request Jun 15, 2023
[skip ci]

## [3.16.1](v3.16.0...v3.16.1) (2023-06-15)

### Bug Fixes

* ipc not work with worker_threads mode ([#5210](#5210)) ([03c8cf7](03c8cf7))
@github-actions
Copy link

🎉 This PR is included in version 3.16.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@fengmk2
Copy link
Member

fengmk2 commented Jun 15, 2023

#5211 测试用例在这里修复

@Wai-Dung
Copy link
Contributor

#5211 测试用例在这里修复

我之前也想这样做,但只是觉得不可思议……为啥必须要一个个而不是异步并行,是有什么文档说明吗?

@fengmk2
Copy link
Member

fengmk2 commented Jun 15, 2023

#5211 测试用例在这里修复

我之前也想这样做,但只是觉得不可思议……为啥必须要一个个而不是异步并行,是有什么文档说明吗?

目前看起来不稳定,还没找到 root cause

@Wai-Dung
Copy link
Contributor

目前看起来不稳定,还没找到 root cause

是的,我反复跑了几次,Windows上都可以通过,太奇怪了。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants