feat: close #7

shaoshuai0102 · 2017-01-26T07:31:41Z

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

popomore · 2017-01-26T11:20:20Z

lib/leader.js

+  close() {
+    this.removeAllListeners();
+    this._server.removeAllListeners();
+    this._server.close();


Server close，follower 也会优雅断开么？

popomore · 2017-01-26T15:55:38Z

那最后调用 client 的 close 就好了么。

如果多个 client，其中一个 client close 了，对应的 server 会关闭么

fengmk2 · 2017-02-03T03:12:50Z

close 应该要支持同步和异步方式，例如有些 close 是需要去删除文件的，那么同步 close 实现不了。

popomore · 2017-02-03T03:22:52Z

默认用 promise 好了

gxcsoccer · 2017-02-03T03:23:01Z

close(callback) 吧

gxcsoccer · 2017-02-03T03:25:34Z

那最后调用 client 的 close 就好了么。
如果多个 client，其中一个 client close 了，对应的 server 会关闭么

各管各吧

popomore · 2017-02-03T04:20:46Z

server 就一个端口，怎么各管各的？

gxcsoccer · 2017-02-03T12:05:55Z

server 就一个端口，怎么各管各的？

如果是多个 client，要关闭所有的，不然剩下的会继续抢占端口。

当然也可以设计一个协议是专门关闭所有 client 的

popomore · 2017-02-03T12:10:19Z

每个 client 的 leader/follower 应该都是通过同一个 server 的端口来通讯的，那这个端口应该什么时候关闭呢？

shaoshuai0102 · 2017-02-03T12:27:40Z

这个地方做起来是比较麻烦的，比较完美的办法是，server close 的时候只是从 typeSet 中移除记录，然后再遍历 typeSet + serverMap，如果发现 typeSet 中对于某个 port 来说找不到 serverMap 中对应监听的了就真正关闭 port。否则在多个应用一起跑的用例里面因为 close 可能会互相影响。

宗羽这块比较绕，我的思路对不

codecov-io · 2017-02-03T14:08:40Z

Codecov Report

Merging #7 into master will increase coverage by 0.31%.

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage    96.2%   96.51%   +0.31%     
==========================================
  Files          15       15              
  Lines         658      718      +60     
==========================================
+ Hits          633      693      +60     
  Misses         25       25

Impacted Files	Coverage Δ
lib/symbol.js	`100% <100%> (ø)`	✅
lib/client.js	`98.8% <100%> (+0.16%)`	✅
lib/index.js	`97.46% <100%> (+0.09%)`	✅
lib/connection.js	`93.05% <100%> (-2.47%)`	❌
lib/server.js	`97.56% <100%> (+2.96%)`	✅
lib/utils.js	`93.93% <100%> (+2.63%)`	✅
lib/leader.js	`94.61% <96.15%> (-0.03%)`	❌

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 503410d...2ad1ec4. Read the comment docs.

popomore · 2017-02-03T14:09:21Z

有点搞混，这个 port 是 client 对外的 port，还是 leader/follower 通讯的 port

shaoshuai0102 · 2017-02-03T14:12:23Z

@popomore 我上面那个回复不对，理解错了。

现在知道怎么做了。当 leader close 的时候，需要：

不再监听实际的 net.Server
解除 cluster name + port 对 net.Server 的独占，允许后面的同 name + port 可以绑定。真正的 net.Server close 暂时可以不做，没有真正的影响。

popomore · 2017-02-03T14:15:19Z

为啥不影响，因为就一个单例？

shaoshuai0102 · 2017-02-03T14:17:08Z

是的，全局有一个 map，对应 port <=> net.Server instance，关不关 server instance 没有实际影响，是因为影响抢占的是另外一个 cluster client name + port 这个组合，只要解除了这个就哦了。

fengmk2 · 2017-02-06T03:35:29Z

@shaoshuai0102 windows 下抛错了 https://ci.appveyor.com/project/gxcsoccer/cluster-client/build/1.0.105/job/ermy2q6shh7ivl77

shaoshuai0102 · 2017-02-06T03:30:55Z

lib/server.js

+    // close server if no one is listening on this port any more
+    if (!listening) {
+      const server = serverMap.get(port);
+      yield server && server.close();


@gxcsoccer 这里做了最后的 close。如果没有 leader 在 listening port 的时候，做最后的关闭。这样就相互不打扰。

gxcsoccer · 2017-02-06T03:38:06Z

@shaoshuai0102 windows 下抛错了 https://ci.appveyor.com/project/gxcsoccer/cluster-client/build/1.0.105/job/ermy2q6shh7ivl77

这个就是 close 的时候还没有 init 导致的

shaoshuai0102 · 2017-02-06T03:43:16Z

这个好奇怪，在 win 下同样在 co 里时序好像不能保证

shaoshuai0102 · 2017-02-06T07:01:35Z

同步讨论结果：

增加 cluster.close(client) API，这样可以取消对 original client close 方法做 delegate 的限制
基于 客户端对等 的原则，使用上应该不区分 leader 和 follower，调整成 leader.close 不会主动通知 follower close，会等待 follower.close 完成。

popomore · 2017-02-06T07:28:00Z

增加 cluster.close(client) API，这样可以取消对 original client close 方法做 delegate 的限制

cluster.close 是哪个对象？单例？

shaoshuai0102 · 2017-02-06T07:29:29Z

单例

const cluster = require('cluster-client');

const client = cluster().create();
cluster.close(client);

popomore · 2017-02-06T07:30:03Z

leader.close 不会主动通知 follower close，会等待 follower.close 完成。

这句不是很懂，不通知咋等待？

leader 是对外的通道，如果 leader 关了，follow 还有存在的必要？

popomore · 2017-02-06T07:31:19Z

这个好像和 client.close 没本质区别？在 client.close 里也可以有相同实现。

shaoshuai0102 · 2017-02-06T07:35:21Z

leader 是对外的通道，如果 leader 关了，follow 还有存在的必要？

没有存在的必要。

follower.close() => 直接关闭
leader.close() => mark 为 closing，等连在 leader 上的所有 follower 都 close 了，真正关闭。如果 follower 一直不关是会导致 leader 一直等待不会关。在 egg 的情况不会存在，因为 egg 中 follower 在 app worker 中会比 agent 进程中的 leader 先关闭。之所以这么设计，是考虑 cluster-client 这个模块本身的理念，所有 client 是对等的，不能因为 close 了其中一个 client 导致其他 client 有异常。

这个机制和原生的 net.Server#close 是比较相似的

shaoshuai0102 · 2017-02-06T07:36:43Z

这个好像和 client.close 没本质区别？在 client.close 里也可以有相同实现。

内部实现是一样的。但是如果 close 方法挂载到 client 上，会导致如果 real client 上有 generator close 方法时，不能 delegate 到 client 上，这里就有潜规则了。如果用 cluster.close 就规避了这个问题。

popomore · 2017-02-06T07:43:52Z

@shaoshuai0102 好的，说的比较清楚了。可以在 egg cluster 模式下测下 close

shaoshuai0102 · 2017-02-06T08:57:56Z

改好了 @gxcsoccer

shaoshuai0102 · 2017-02-07T02:20:03Z

合并？

gxcsoccer · 2017-02-07T02:21:59Z

@shaoshuai0102 再看一遍

shaoshuai0102 · 2017-02-07T02:22:42Z

好 egg 1.0 在等这个

gxcsoccer · 2017-02-07T02:27:15Z

lgtm

gxcsoccer

+1

gxcsoccer · 2017-02-07T02:29:03Z

lib/leader.js

+          });
+        }
+
+        setTimeout(reject, 30000);


reject 应该有一个 err 吧？

shaoshuai0102 · 2017-02-07T03:12:07Z

ci 过了

合并发布后 eggjs/egg#310 就可以过了

gxcsoccer · 2017-02-07T03:15:46Z

+ [email protected]

shaoshuai0102 requested review from fengmk2, popomore and gxcsoccer January 26, 2017 07:31

shaoshuai0102 mentioned this pull request Jan 26, 2017

cluster-client 的一些问题 eggjs/egg#304

Closed

popomore reviewed Jan 26, 2017

View reviewed changes

shaoshuai0102 force-pushed the feat-close branch from 16bff7e to 628cc32 Compare February 3, 2017 11:27

shaoshuai0102 changed the title ~~feat: close~~ [WIP] feat: close Feb 3, 2017

shaoshuai0102 mentioned this pull request Feb 3, 2017

feat: close clust clients before close eggjs/egg#310

Merged

4 tasks

shaoshuai0102 force-pushed the feat-close branch 6 times, most recently from f224193 to f4cf083 Compare February 3, 2017 14:08

shaoshuai0102 force-pushed the feat-close branch from f4cf083 to 3023b7c Compare February 3, 2017 14:14

shaoshuai0102 force-pushed the feat-close branch from 3023b7c to 493e112 Compare February 3, 2017 14:51

shaoshuai0102 commented Feb 6, 2017

View reviewed changes

shaoshuai0102 force-pushed the feat-close branch from 730ed09 to f783127 Compare February 6, 2017 03:57

shaoshuai0102 force-pushed the feat-close branch from f783127 to dd07415 Compare February 6, 2017 07:18

shaoshuai0102 force-pushed the feat-close branch from dd07415 to 64b4e5d Compare February 6, 2017 08:32

gxcsoccer approved these changes Feb 7, 2017

View reviewed changes

gxcsoccer reviewed Feb 7, 2017

View reviewed changes

lib/leader.js Outdated

});

}

setTimeout(reject, 30000);

Copy link

Member

gxcsoccer Feb 7, 2017

Choose a reason for hiding this comment

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

reject 应该有一个 err 吧？

shaoshuai0102 force-pushed the feat-close branch from 64b4e5d to cd173ce Compare February 7, 2017 02:34

feat: close

2ad1ec4

shaoshuai0102 force-pushed the feat-close branch from cd173ce to 2ad1ec4 Compare February 7, 2017 02:39

gxcsoccer merged commit 00e6f2f into master Feb 7, 2017

gxcsoccer deleted the feat-close branch February 7, 2017 03:18

atian25 mentioned this pull request Feb 7, 2017

知乎：如何评价阿里开源的企业级 Node.js 框架 egg？ atian25/blog#18

Open

feat: close #7

feat: close #7

Conversation

shaoshuai0102 commented Jan 26, 2017 • edited by gxcsoccer Loading

Checklist

Affected core subsystem(s)

Description of change

popomore Jan 26, 2017

Choose a reason for hiding this comment

popomore commented Jan 26, 2017

fengmk2 commented Feb 3, 2017

popomore commented Feb 3, 2017

gxcsoccer commented Feb 3, 2017

gxcsoccer commented Feb 3, 2017

popomore commented Feb 3, 2017

gxcsoccer commented Feb 3, 2017

popomore commented Feb 3, 2017 • edited Loading

shaoshuai0102 commented Feb 3, 2017 • edited Loading

codecov-io commented Feb 3, 2017 • edited Loading

Codecov Report

popomore commented Feb 3, 2017

shaoshuai0102 commented Feb 3, 2017

popomore commented Feb 3, 2017

shaoshuai0102 commented Feb 3, 2017

fengmk2 commented Feb 6, 2017

shaoshuai0102 Feb 6, 2017

Choose a reason for hiding this comment

gxcsoccer commented Feb 6, 2017

shaoshuai0102 commented Feb 6, 2017

shaoshuai0102 commented Feb 6, 2017 • edited Loading

popomore commented Feb 6, 2017

shaoshuai0102 commented Feb 6, 2017

popomore commented Feb 6, 2017

popomore commented Feb 6, 2017

shaoshuai0102 commented Feb 6, 2017

shaoshuai0102 commented Feb 6, 2017

popomore commented Feb 6, 2017

shaoshuai0102 commented Feb 6, 2017

shaoshuai0102 commented Feb 7, 2017

gxcsoccer commented Feb 7, 2017

shaoshuai0102 commented Feb 7, 2017

gxcsoccer commented Feb 7, 2017

gxcsoccer left a comment

Choose a reason for hiding this comment

gxcsoccer Feb 7, 2017

Choose a reason for hiding this comment

shaoshuai0102 commented Feb 7, 2017

gxcsoccer commented Feb 7, 2017

shaoshuai0102 commented Jan 26, 2017 •

edited by gxcsoccer

Loading

popomore commented Feb 3, 2017 •

edited

Loading

shaoshuai0102 commented Feb 3, 2017 •

edited

Loading

codecov-io commented Feb 3, 2017 •

edited

Loading

shaoshuai0102 commented Feb 6, 2017 •

edited

Loading