search

baidu / sofa-pbrpc

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

Add timeout to connect action #77

Closed yuandong1222 closed 8 years ago

yuandong1222 commented 8 years ago

A new option of rpc_controller connect_timeout is used for connect action.

When rpc_client use rpc_stream to create a connect, it will use rpc_controller::connect_timeout for connect action if it is > 0.

The default of rpc_controller::connect_timeout is -1.

Signed-off-by: Dong Yuan yuandong1222@gmail.com

qinzuoyan commented 8 years ago

I think ConnectionTimeout is not a Request level options, it's better to put the option in RpcClientOptions

yuandong1222 commented 8 years ago

I impl this opinion as a RpcController's member since connecting to different may need different timeout.

qinzuoyan commented 8 years ago

But in fact, the connection is transparent to user, and all rpc channels using the same RpcClient will share the same connections, which means, even for different channels, if their remote addresses (ip:port to be exact) are the same, they will use the same underlying connection. So, setting connection timeout in RpcController is not proper, because it may not take effect when the connection is setting up by another RpcChannel.

yuandong1222 commented 8 years ago

Yes, this make sense, I will fix it later.

3ks.

qinzuoyan commented 8 years ago

Nice, thanks~

yuandong1222 commented 8 years ago

@cyshi, Yes. There is bug in this patch which I fixed and now am testing.

Please don't merge.

yuandong1222 commented 8 years ago

Updated, plz review.

qinzuoyan commented 8 years ago

@yuandong1222 , is the bug you mentioned several days ago fixed now? and no core appeared any more?

yuandong1222 commented 8 years ago

@qinzuoyan Yes, I fix it. This is because I bind with this not _shared_fromthis.

This path is used on our online services which has more than 1k nodes now and everything runs fine, I think it is OK.

qinzuoyan commented 8 years ago

Nice, merged, thanks for your contribution.

cyshi commented 8 years ago

@yuandong1222 方便的话可以补充一下README中的User List