feat: supports serialization of options #97

waitingsong · 2020-02-13T12:16:14Z

ref: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

Checklist

npm test passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Description of change

ref: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

waitingsong · 2020-02-13T12:17:11Z

Node.js 12.16 支持高级序列化方式
https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

codecov · 2020-02-13T12:44:08Z

Codecov Report

Merging #97 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   98.82%   98.83%   +0.01%     
==========================================
  Files           8        8              
  Lines         511      517       +6     
==========================================
+ Hits          505      511       +6     
  Misses          6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 423726d...1249d64. Read the comment docs.

waitingsong · 2020-02-13T13:30:32Z

serialization 在 Node.js < 12.16 时可以传递任何值，不会异常。在 >= 12.16 时非 json advanced 值会抛异常。

另，是否需要在 debug 模式时默认设置为 advanced 值？
或者当 >= 12.16.0 时全部默认 'advanced'？

ref: eggjs/cluster#97

hyj1991 · 2020-02-14T01:38:24Z

README.md

+| https           | `Object`        | start a https server, note: `key` / `cert` / `ca` should be full path to file |
+| require         | `Array\|String` | will inject into worker/agent process                                         |
+| pidFile         | `String`        | will save master pid to this file                                             |
+| [serialization] | `json|advanced` | default: 'json'. 'advanced' requires Node.js >= 10.16.0                       |



这里是 12.16.0 吧？

另，是否需要在 debug 模式时默认设置为 advanced 值？
或者当 >= 12.16.0 时全部默认 'advanced'？

我看了下下面的相关讨论：nodejs/node#30162 (comment), 看起来 advanced 并不是所有场景下都是最优解，所以这里确实应该 follow 上游的设置，默认为 'json'，但是在框架里在适当的版本暴露可配置选项。

我也是如此考虑：随着版本更新性能提升及可靠性得到进一步验证，可能在未来某个版本默认改为 advanced。
所以跟随 node.js 默认设置（json），代码里面不做默认值设置，让有需要的用户自行决定传入参数控制。

README.md

hyj1991 · 2020-02-14T01:42:40Z

lib/master.js

@@ -248,6 +250,14 @@ class Master extends EventEmitter {
    const debugPort = process.env.EGG_AGENT_DEBUG_PORT || 5800;
    if (this.options.isDebug) opt.execArgv = process.execArgv.concat([ `--${semver.gte(process.version, '8.0.0') ? 'inspect' : 'debug'}-port=${debugPort}` ]);

+    /* istanbul ignore next */
+    if (semver.gte(process.version, '12.16.0')) {


这里的判断有一个问题，'v13.0.0' 也是 >= 12.16.0，但是这个版本 patch 了此功能么

对于 12 版本是 >= 12.16.0，对于 13 版本是 >= 13.2.0。
代码已更新判断条件。

lib/utils/options.js

hyj1991

感谢您关于 v12.16.0 更新的支持更多 IPC 序列/反序列化的配置参数 PR，以上是我的一些对于 PR 代码的修改建议，期望可以一起针对修改/不修改的合理性进行讨论 :)

hyj1991 · 2020-02-14T02:02:23Z

另，是否需要在 debug 模式时默认设置为 advanced 值？
或者当 >= 12.16.0 时全部默认 'advanced'？

我看了下下面的相关讨论：nodejs/node#30162 (comment), 看起来 advanced 并不是所有场景下都是最优解，所以这里确实应该 follow 上游的设置，默认为 'json'，但是在框架里在适当的版本暴露可配置选项。

atian25 · 2020-02-14T02:28:10Z

如果这个未来还会变化的话，那目前能否先通过 NODE_OPTIONS ENV 的方式来使用就好了？

>= v12.16.0 < v13.0.0, or >= v13.2.0

waitingsong · 2020-02-14T02:31:29Z

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

atian25 · 2020-02-14T03:07:15Z

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的，fork 默认是继承 env 的。可以试验下

waitingsong · 2020-02-14T03:43:24Z

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的，fork 默认是继承 env 的。可以试验下

就是说不修改代码，通过设置 ENV 也能起效果。那我关闭这个 PR 了哟？

atian25 · 2020-02-14T03:44:13Z

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的，fork 默认是继承 env 的。可以试验下

就是说不修改代码，通过设置 ENV 也能起效果。那我关闭这个 PR 了哟？

我理解是这样的，你实验下看看

waitingsong · 2020-02-14T03:46:38Z

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的，fork 默认是继承 env 的。可以试验下

就是说不修改代码，通过设置 ENV 也能起效果。那我关闭这个 PR 了哟？

我理解是这样的，你实验下看看

那我测试下。

hyj1991 · 2020-02-18T03:51:36Z

所以这个 PR 还有下文不。。。

waitingsong · 2020-02-18T11:41:53Z

所以这个 PR 还有下文不。。。

么时间测试啊，我先关掉。

feat: supports serialization of options

59e8612

ref: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

atian25 requested a review from hyj1991 February 13, 2020 12:22

chore: fix style

8aa9fc2

chore: move option.serialization of validation into utils/options.js

23fc7b3

waitingsong force-pushed the child_process branch from 96f73a0 to 23fc7b3 Compare February 13, 2020 12:52

waitingsong force-pushed the child_process branch from 120bebe to 0eba33b Compare February 13, 2020 13:34

waitingsong added a commit to waitingsong/egg that referenced this pull request Feb 13, 2020

feat(typings): add ClusterOptions['serialization']

3a0958f

ref: eggjs/cluster#97

waitingsong mentioned this pull request Feb 13, 2020

feat(typings): add ClusterOptions['serialization'] eggjs/egg#4182

Closed

4 tasks

waitingsong force-pushed the child_process branch from 0eba33b to e9d9069 Compare February 13, 2020 14:05

refactor: update validation of options.serialization

9d9f33a

waitingsong force-pushed the child_process branch from e9d9069 to 9d9f33a Compare February 13, 2020 14:57

chore: update parameter comments of class Master

9727bf3

hyj1991 reviewed Feb 14, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

hyj1991 reviewed Feb 14, 2020

View reviewed changes

lib/utils/options.js Outdated Show resolved Hide resolved

chore: clean README.md

097dd0a

hyj1991 requested changes Feb 14, 2020

View reviewed changes

feat: update Node.js version requirement for serialization

1249d64

>= v12.16.0 < v13.0.0, or >= v13.2.0

waitingsong force-pushed the child_process branch from 4179e83 to 1249d64 Compare February 14, 2020 02:28

waitingsong closed this Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: supports serialization of options #97

feat: supports serialization of options #97

waitingsong commented Feb 13, 2020 •

edited

Loading

waitingsong commented Feb 13, 2020

codecov bot commented Feb 13, 2020 •

edited

Loading

waitingsong commented Feb 13, 2020 •

edited

Loading

hyj1991 Feb 14, 2020

waitingsong Feb 14, 2020 •

edited

Loading

hyj1991 Feb 14, 2020

waitingsong Feb 14, 2020

hyj1991 left a comment

hyj1991 commented Feb 14, 2020 •

edited

Loading

atian25 commented Feb 14, 2020

waitingsong commented Feb 14, 2020 •

edited

Loading

atian25 commented Feb 14, 2020

waitingsong commented Feb 14, 2020

atian25 commented Feb 14, 2020

waitingsong commented Feb 14, 2020

hyj1991 commented Feb 18, 2020 •

edited

Loading

waitingsong commented Feb 18, 2020

feat: supports serialization of options #97

feat: supports serialization of options #97

Conversation

waitingsong commented Feb 13, 2020 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

waitingsong commented Feb 13, 2020

codecov bot commented Feb 13, 2020 • edited Loading

Codecov Report

waitingsong commented Feb 13, 2020 • edited Loading

hyj1991 Feb 14, 2020

Choose a reason for hiding this comment

waitingsong Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

hyj1991 Feb 14, 2020

Choose a reason for hiding this comment

waitingsong Feb 14, 2020

Choose a reason for hiding this comment

hyj1991 left a comment

Choose a reason for hiding this comment

hyj1991 commented Feb 14, 2020 • edited Loading

atian25 commented Feb 14, 2020

waitingsong commented Feb 14, 2020 • edited Loading

atian25 commented Feb 14, 2020

waitingsong commented Feb 14, 2020

atian25 commented Feb 14, 2020

waitingsong commented Feb 14, 2020

hyj1991 commented Feb 18, 2020 • edited Loading

waitingsong commented Feb 18, 2020

waitingsong commented Feb 13, 2020 •

edited

Loading

codecov bot commented Feb 13, 2020 •

edited

Loading

waitingsong commented Feb 13, 2020 •

edited

Loading

waitingsong Feb 14, 2020 •

edited

Loading

hyj1991 commented Feb 14, 2020 •

edited

Loading

waitingsong commented Feb 14, 2020 •

edited

Loading

hyj1991 commented Feb 18, 2020 •

edited

Loading