fix ssl state exception coredump

[chenBright]

fix #658

coredump case的场景：当客户端Controller::IssueRPC选出节点后、Socket::Write KEEPWRITE_IN_BACKGROUND前，服务端下线了，客户端在OnNewMwssage中读到EOF，将Socket SetFailed，并启动了HealthCheckThread。此时，socket的引用计数为3（Controller::IssueRPC、HealthCheckThread、SocketMap），HealthCheckThread在WaitAndRest等引用计数变成2（预期是HealthCheckThread、SocketMap）。然后，NamingService将该下游节点摘掉，并将对应socket从SocketMap中删除，引用计数减一，变成2。HealthCheckThread在WaitAndRest中将_ssl_state置为SSL_UNKONW。最后，客户端Controller::IssueRPC走到Socket::KeepWrite。虽然客户端未开启SSL，但是HealthCheckTask将_ssl_state置为SSL_UNKONW了，所以程序会运行到写SSL（空指针）的逻辑，最终导致coredump。

解决方案：设置flag标识是否停止health check thread。当SocketMap删除socket时，先将flag置为true，再释放引用计数。HealthCheckThread中Wait到2之后，判断改flag是否为true。是，则直接返回，退出health check thread。否则，继续进行health check。

[chenBright]

@wwbmmm 请问一下，为什么ci没有触发？

[wwbmmm]

@wwbmmm 请问一下，为什么ci没有触发？

可能是github的bug，重新push一下试试

[wwbmmm]

LGTM， @zyearn 有空也看看

[chenBright]

@zyearn 还有什么问题吗？大概什么时候能合呢？

[zyearn]

能不能把堆栈发一下？是不是core在了https://github.com/apache/incubator-brpc/blob/master/src/brpc/socket.cpp?#L1692？

[chenBright]

@zyearn 和 #658 一样。因为ssl为NULL，所以core在cut_multiple_into_SSL_channel函数里。

[zyearn]

@chenBright 这个问题能不能通过在判断ssl_state和_ssl_session基础上解决呢？比如在DoWrite里

current_ssl_session = _ssl_session
if (current_ssl_session == NULL) {
   // necessary comment
   return -1;
}
...
butil::IOBuf::cut_multiple_into_SSL_channel(current_ssl_session, ...)

这样保证current_ssl_session不是NULL，不出core，然后写fd的时候发现对端已经关了连接就走正常错误处理的代码了。

[chenBright]

@chenBright 这个问题能不能通过在判断ssl_state和_ssl_session基础上解决呢？比如在DoWrite里

current_ssl_session = _ssl_session
if (current_ssl_session == NULL) {
   // necessary comment
   return -1;
}
...
butil::IOBuf::cut_multiple_into_SSL_channel(current_ssl_session, ...)

这样保证current_ssl_session不是NULL，不出core，然后写fd的时候发现对端已经关了连接就走正常错误处理的代码了。

@zyearn 这样可以保证不会coredump，这也是正常的边界处理逻辑。当时有考虑通过判断_ssl_session或者fd来解决，但是觉得并没有实际解决问题。现在是HealthCheckTask里的引用不符合预期，导致了Socket内部的状态错误的问题，这样处理的话，会不会不太合适呢？

[chenBright]

@chenBright 这个问题能不能通过在判断ssl_state和_ssl_session基础上解决呢？比如在DoWrite里

current_ssl_session = _ssl_session
if (current_ssl_session == NULL) {
   // necessary comment
   return -1;
}
...
butil::IOBuf::cut_multiple_into_SSL_channel(current_ssl_session, ...)

这样保证current_ssl_session不是NULL，不出core，然后写fd的时候发现对端已经关了连接就走正常错误处理的代码了。

还有一个问题：#658 这个case中，当HealthCheckTask等到引用计数为2后，Connect之前，另一处rpc DoWrite失败，rpc结束，引用计数减一。此时，该下游节点恢复了，HealthCheckTask成功Connect，并将Socket Revive。此时Socket是正常的，但是没有地方持有对应的SocketId，那么这个Socket是“泄漏”了吧？

[chenBright]

@zyearn 请问一下，还有什么其他解决方案或者建议吗？最近一个月内有几个服务因为这个问题导致coredump了，麻烦有空推进一下呢。

[zyearn]

@chenBright 这个问题能不能通过在判断ssl_state和_ssl_session基础上解决呢？比如在DoWrite里

current_ssl_session = _ssl_session
if (current_ssl_session == NULL) {
   // necessary comment
   return -1;
}
...
butil::IOBuf::cut_multiple_into_SSL_channel(current_ssl_session, ...)

这样保证current_ssl_session不是NULL，不出core，然后写fd的时候发现对端已经关了连接就走正常错误处理的代码了。

@zyearn 这样可以保证不会coredump，这也是正常的边界处理逻辑。当时有考虑通过判断_ssl_session或者fd来解决，但是觉得并没有实际解决问题。现在是HealthCheckTask里的引用不符合预期，导致了Socket内部的状态错误的问题，这样处理的话，会不会不太合适呢？

Sorry看到晚了，加了一条comment，麻烦看下。总体我觉得没有什么问题，在增加代码的地方加点注释解释为什么这么做就好了。

[zyearn]

@chenBright 这个问题能不能通过在判断ssl_state和_ssl_session基础上解决呢？比如在DoWrite里

current_ssl_session = _ssl_session
if (current_ssl_session == NULL) {
   // necessary comment
   return -1;
}
...
butil::IOBuf::cut_multiple_into_SSL_channel(current_ssl_session, ...)

这样保证current_ssl_session不是NULL，不出core，然后写fd的时候发现对端已经关了连接就走正常错误处理的代码了。

还有一个问题：#658 这个case中，当HealthCheckTask等到引用计数为2后，Connect之前，另一处rpc DoWrite失败，rpc结束，引用计数减一。此时，该下游节点恢复了，HealthCheckTask成功Connect，并将Socket Revive。此时Socket是正常的，但是没有地方持有对应的SocketId，那么这个Socket是“泄漏”了吧？

不会，HealthCheckTask持有一个引用，因为只有2个引用，rpc结束减1，HealthCheckTask结束减1，这时候socket就被回收了

[chenBright]

@chenBright 这个问题能不能通过在判断ssl_state和_ssl_session基础上解决呢？比如在DoWrite里

current_ssl_session = _ssl_session
if (current_ssl_session == NULL) {
   // necessary comment
   return -1;
}
...
butil::IOBuf::cut_multiple_into_SSL_channel(current_ssl_session, ...)

这样保证current_ssl_session不是NULL，不出core，然后写fd的时候发现对端已经关了连接就走正常错误处理的代码了。

@zyearn 这样可以保证不会coredump，这也是正常的边界处理逻辑。当时有考虑通过判断_ssl_session或者fd来解决，但是觉得并没有实际解决问题。现在是HealthCheckTask里的引用不符合预期，导致了Socket内部的状态错误的问题，这样处理的话，会不会不太合适呢？

Sorry看到晚了，加了一条comment，麻烦看下。总体我觉得没有什么问题，在增加代码的地方加点注释解释为什么这么做就好了。

已修改

[zyearn]

有一个问题：这个PR为什么会修改selective_channel？在socket_map.cpp中增删前设置in_socket_map是不是就足够了？

selective_channel里sub_channel中的Naming service会在对端离开的时候通过SocketMapRemove将对端删除，从而间接影响selective_channel了。

[chenBright]

有一个问题：这个PR为什么会修改selective_channel？在socket_map.cpp中增删前设置in_socket_map是不是就足够了？

selective_channel里sub_channel中的Naming service会在对端离开的时候通过SocketMapRemove将对端删除，从而间接影响selective_channel了。

SocketMapRemove 删除的是sub_channel中的Naming service的socket吧，而这里的socket是selective_channel里为每个sub_channel创建对应的fake socket，用于lb选路。ChannelBalancer::AddChannel中创建fake socket的时候设置了health_check_interval_s，预期是fake socket被SetFailed了，启动健康检查机制，对fake socket的SocketUser（在selective_channel中设置为sub_channel）进行健康检查。

如果不修改selective_channel，fake socket的_is_in_socket_map为false，那么fake socket的健康检查机制实际上不会执行了。虽然目前只看到ChannelBalancer::AddChannel中AddServer失败的时候才会将该socket SetFailed，随后该socket就会被回收了，健康检查执行与否都没影响，但是还是影响了现有的逻辑，感觉不合理，需要修改。

[chenBright]

不管是来自于SocketMap还是selective_channel中的_chan_map，健康检查机制关注的还是引用。将_is_in_socket_map改成_is_hc_related_ref_held，是不是更合适一点？_is_hc_related_ref_held表示外部持有了一个与健康检查机制有关的引用，持有该引用的一部分作用是让健康检查机制正常执行。这样就不会与SocketMap耦合了，在selective_channel中变成设置_is_hc_related_ref_held也自然多了。

[zyearn]

不管是来自于SocketMap还是selective_channel中的_chan_map，健康检查机制关注的还是引用。将_is_in_socket_map改成_is_hc_related_ref_held，是不是更合适一点？_is_hc_related_ref_held表示外部持有了一个与健康检查机制有关的引用，持有该引用的一部分作用是让健康检查机制正常执行。这样就不会与SocketMap耦合了，在selective_channel中变成设置_is_hc_related_ref_held也自然多了。

好的，这个名字可以。另外可以检查一下其它combo channel有没有类似的问题？

[chenBright]

不管是来自于SocketMap还是selective_channel中的_chan_map，健康检查机制关注的还是引用。将_is_in_socket_map改成_is_hc_related_ref_held，是不是更合适一点？_is_hc_related_ref_held表示外部持有了一个与健康检查机制有关的引用，持有该引用的一部分作用是让健康检查机制正常执行。这样就不会与SocketMap耦合了，在selective_channel中变成设置_is_hc_related_ref_held也自然多了。

好的，这个名字可以。另外可以检查一下其它combo channel有没有类似的问题？

已修改。确认过了，其它combo channel没有类似的问题。

[zyearn]

多谢贡献

[ChinaChenp]

我们刚好也遇到这个问题，居然已经修复了，感谢大佬们的贡献；

[pavel2003]

我们有个环境也有类似的问题，但是我们没有使用nameservice，channel都是ip port直接连接的，这个会是什么问题

@chenBright

[pavel2003]

[chenBright]

用ip:port直连，也是会有同样问题的。你们的brpc已经带上这个patch了吗？@pavel2003

[pavel2003]

还没带，我想先根据这个流程复现下，我在KeepWrite之前先sleep了，但是HealthCheckThread会在WaitAndReset中一直等

因为没有NamingService的话，应该没人从SocketMap把socket删掉吧， 我们的channel是全局的，channel自己不会释放

@chenBright

[chenBright]

全局的channel就不会删除SocketMap的socket了，那应该不是这个问题导致的了。