baidu / sofa-pbrpc

A light-weight RPC implement of google protobuf RPC framework.
Other
2.13k stars 655 forks source link

remove Shutdown interface from RpcClient #139

Closed cyshi closed 7 years ago

cyshi commented 7 years ago

RpcClient提供给用户的接口应该是线程安全的,用户使用Shutdown接口会导致RpcClientImpl中的成员变量析构,如果有其他线程正在使用RpcClient,会导致coredump

另外从语义上看,Start是在RcpClient的构造函数中暴露给用户的,单独暴露一个Shutdonw接口给用户,应该不是必要的

@qinzuoyan @zd-double @bluebore

zd-double commented 7 years ago

感觉Shutdown直观的语义只是停止工作,但是client的Shutdown会对client的thread_group、timeout_manager等成员析构。用户可能会因为不慎调用Shutdown,导致其他操作client的线程core掉。所以去掉这个接口比较合理。

qinzuoyan commented 7 years ago

其实这里当时有一个考虑是优雅退出:如果用户使用了一个全局的RpcClient,该对象只有在进程退出的时候才会析构,此时才会释放thre ad_group、timeout_manager等资源;在此之前RpcClient所持有的线程都在运行中,如果线程所依赖的其他对象(譬如通过Closure传给RpcClient)提前析构了(因为全局对象的析构顺序是undefined的),那么很可能出现coredump等不期望的事情发生。

当然Shutdown()不暴露给用户的话,用户就需要自己控制析构函数执行的点,譬如全局变量放的是指针,然后通过new/delete来创建和销毁RpcClient对象。

另外关于“用户不慎调用Shutdown()”感觉没法很好解决。如果他要使用该函数,就应当清楚对象的生命周期,不然就算这里给他规避了,其他地方他也很容易犯错,譬如使用Closure的时候。

On Sat, Nov 5, 2016 at 7:37 PM, ZhangDi notifications@github.com wrote:

感觉Shutdown直观的语义只是停止工作,但是client的Shutdown会对client的thre ad_group、timeout_manager等成员析构。用户可能会因为不慎调用Shutdown,导致其他操作client的线程core掉。 所以去掉这个接口比较合理。

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/baidu/sofa-pbrpc/issues/139#issuecomment-258606262, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgQPfCK1dPxeJcaBH1YDFb75xTKQQL-ks5q7GpwgaJpZM4KqO-H .

cyshi commented 7 years ago

现在看用户挺难知道RpcClient的Shutdown()会释放其成员对象,这部分代码是在RpcClient实现中,用户通过头文件比较难判断

暂时可以先完善下注释,各位看看

qinzuoyan commented 7 years ago

恩,完善下注释吧

On Mon, Nov 7, 2016 at 9:59 AM, cyshi notifications@github.com wrote:

现在看用户挺难知道RpcClient的Shutdown()会释放其成员对象,这部分代码是在RpcClient实现中,用户通过头文件比较难判断

暂时可以先完善下注释,各位看看

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/baidu/sofa-pbrpc/issues/139#issuecomment-258731066, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgQPRAD1uTHPpVZi_oI99axIeRzfmsSks5q7oYOgaJpZM4KqO-H .