fix memory leak issue #2871 #2872
Conversation
|
This PR caused the BuiltinServiceTest.customized_health unit test to fail. |
src/brpc/server.cpp
Outdated
| if (!_options.rpc_pb_message_factory) { | ||
| _options.rpc_pb_message_factory = new DefaultRpcPBMessageFactory(); | ||
| } |
There was a problem hiding this comment.
有这里的逻辑，Server里赋的默认值还有必要吗？
There was a problem hiding this comment.
有的，有点防不胜防的意味。
例如brpc_channel_unittest.cpp:260:
brpc::RpcPBMessages* messages =
ts->_dummy.options().rpc_pb_message_factory->Get(ts->_svc, *method);_dummy是一个brpc::Server对象，其初始化后并没有手动创建rpc_pb_message_factory，最后导致crush。
如果将‘Server里赋的默认值’删除掉，那么所有类似于_dummy的用法都是需要修改的，这样是否会破坏兼容性？此处不确定这样的破坏是否可接受，故保留了在Server里赋的默认值。
There was a problem hiding this comment.
ok。是不是可以在430行加注释说明一下_dummy这种用法？
There was a problem hiding this comment.
仔细斟酌了一下，server启动前，不保证server.options().rpc_pb_message_factory有效的。brpc_channel_unittest的也是在server启动之后使用的，UT得适应Server的实现。所以，我认为Server构造函数里的初始化是没有必要的。
There was a problem hiding this comment.
仔细斟酌了一下，server启动前，不保证server.options().rpc_pb_message_factory有效的。brpc_channel_unittest的也是在server启动之后使用的，UT得适应Server的实现。所以，我认为Server构造函数里的初始化是没有必要的。
@w-gc 也看看这个
There was a problem hiding this comment.
这个我尝试了不太好改，可能是我对这个Server的使用流程不太熟悉，不太清楚UT里有server但没有启动情况下怎么使用rpc_pb_message_factory：
const google::protobuf::MethodDescriptor* method =
ts->_svc.descriptor()->FindMethodByName(req_meta.method_name());
// 假启动，只为更新options
if (!ts->_dummy.options().rpc_pb_message_factory) {
ts->_dummy.Start(ts->_ep, &ts->_dummy.options());
ts->_dummy.Stop(0);
}
brpc::RpcPBMessages* messages =
ts->_dummy.options().rpc_pb_message_factory->Get(ts->_svc, *method);还是
const google::protobuf::MethodDescriptor* method =
ts->_svc.descriptor()->FindMethodByName(req_meta.method_name());
auto options = ts->_dummy.options();
if (!options.rpc_pb_message_factory) {
options.rpc_pb_message_factory = new brpc::DefaultRpcPBMessageFactory();
}
brpc::RpcPBMessages* messages =
options.rpc_pb_message_factory->Get(ts->_svc, *method);我建议，我这个PR先不改，后续你们来改，或者单独起个PR解决这个问题？毕竟这个语义的变更是会影响到用户的。
There was a problem hiding this comment.
在ChannelTest()加上ts->_dummy.options().rpc_pb_message_factory = new brpc::DefaultRpcPBMessageFactory();应该就行了。
src/brpc/server.cpp
Outdated
| // Don't forget to release `rpc_pb_message_factory` before overwriting `_options` | ||
| delete _options.rpc_pb_message_factory; | ||
| _options.rpc_pb_message_factory = NULL; | ||
|
|
There was a problem hiding this comment.
这个分支，也是用copy_server_option函数，感觉更好些，里面有处理delete pb_message_factory的逻辑了。拷贝的是默认值。
src/brpc/server.cpp
Outdated
| // Always reset to default options explicitly since `_options' | ||
| // may be the options for the last run or even bad options | ||
| _options = ServerOptions(); | ||
| } | ||
|
|
||
| // Create the resource if: |
There was a problem hiding this comment.
这个逻辑也放到copy_server_options是否合理呢？看上去无论是构造，还是后期的设置，rpc_pb_message_factory都是必须不能为空的。
| @@ -424,7 +424,16 @@ Server::Server(ProfilerLinker) | |||
| , _eps_bvar(&_nerror_bvar) | |||
| , _concurrency(0) | |||
| , _concurrency_bvar(cast_no_barrier_int, &_concurrency) | |||
| ,_has_progressive_read_method(false) { | |||
| , _has_progressive_read_method(false) { | |||
There was a problem hiding this comment.
构造这块也是用copy_server_option来初始化_options，这个关于ServerOptions无论是在构造的时候，还是在后期重新设置的时候，都统一了。
What problem does this PR solve?
Issue Number: #2871
Problem Summary:
What is changed and the side effects?
Changed:
Side effects:
Performance effects(性能影响):
Breaking backward compatibility(向后兼容性):
Check List: