grpc / grpc-swift

The Swift language implementation of gRPC.
Apache License 2.0
2.04k stars 420 forks source link

Best Practice regarding ClientConnection #1341

Open Banck opened 2 years ago

Banck commented 2 years ago

Hello! I always used one ClientConnection instance in whole app, but recently faced with case, which made me think that I'm doing wrong.

For example, we have two requests. The first one fails with 404 error, the second one resolves with success response. Everything is fine, but if these two requests will be send at the same time, the second one will be failed with GRPCStatus == 14(unavailable). So seems the first request dropped all other requests.

if create ClientConnection for every request - everything is fine. So do we need to create ClientConnection instance for every request?

Thank you!

PeqNP commented 2 years ago

Your question actually helped me resolve a long standing issue I had with memory consumption and performance within my app. Thank you!

I now cache the ClientConnection but re-create a Client for every new request. It seems like it degrades performance slightly, but better than re-creating the ClientConnection for every request.

Can someone on the team confirm that this is best practice?

It's almost as if request/response objects are not released until the Client object is de-alloced. It also causes some very strange UIKit issues. For instance, button taps become delayed on subsequent calls to the same instance of Client. Very weird!

glbrntt commented 2 years ago

if create ClientConnection for every request - everything is fine. So do we need to create ClientConnection instance for every request?

Generally speaking ClientConnection should be long lived, you should not create one per request.

For example, we have two requests. The first one fails with 404 error, the second one resolves with success response. Everything is fine, but if these two requests will be send at the same time, the second one will be failed with GRPCStatus == 14(unavailable). So seems the first request dropped all other requests.

I think you've hit the unfortunate timing window in which the connection has failed but the connection does not yet know it (and thinks it can handle the second request). The GRPCChannelPool handles this better.


I now cache the ClientConnection but re-create a Client for every new request. It seems like it degrades performance slightly, but better than re-creating the ClientConnection for every request.

Can someone on the team confirm that this is best practice?

Clients are thin wrappers around a GRPCChannel (in this case a ClientConnection) and should not need to be created per request.

It's almost as if request/response objects are not released until the Client object is de-alloced. It also causes some very strange UIKit issues. For instance, button taps become delayed on subsequent calls to the same instance of Client. Very weird!

Can you elaborate on this?

Skoti commented 1 year ago

One of the comments in the mentioned issue says:

For what it's worth, you should prefer using the GRPCChannelPool over ClientConnection. It is a little more resilient to failures.

Currently, there is no information on which way of creating the channel is preferred and the reasoning behind it.

  1. Are there any differences between the two?
  2. Does GRPCChannelPool has some disadvantages?

One noticeable difference is that ClientConnection does not immediately throw errors for TLS configuration while the GRPCChannelPool does. Is there a reason behind it?

From the code perspective it seems like it's not necessary?: as in the DefaultChannelProvider the configureWithNIOSSL is storing the result of makeNIOSSLContext: https://github.com/grpc/grpc-swift/blob/4ab02e1ae5b4dfdd723773e955b62f35ccbaa7c7/Sources/GRPC/ConnectionManagerChannelProvider.swift#L110-L114 and in the PooledChannel the configureWithNIOSSL is storing only the success case: https://github.com/grpc/grpc-swift/blob/4ab02e1ae5b4dfdd723773e955b62f35ccbaa7c7/Sources/GRPC/ConnectionPool/PooledChannel.swift#L45-L48 even though as part of the same initializer code, it creates the DefaultChannelProvider with this tlsMode and all other parameters taken from the configuration: https://github.com/grpc/grpc-swift/blob/4ab02e1ae5b4dfdd723773e955b62f35ccbaa7c7/Sources/GRPC/ConnectionPool/PooledChannel.swift#L81-L85 and the DefaultChannelProvider is capable of elegantly handling the result from configureWithNIOSSL in the makeChannel function anyway: https://github.com/grpc/grpc-swift/blob/4ab02e1ae5b4dfdd723773e955b62f35ccbaa7c7/Sources/GRPC/ConnectionManagerChannelProvider.swift#L207-L209 as it is the case for the ConnectionManager that is created by the ClientConnection: https://github.com/grpc/grpc-swift/blob/4ab02e1ae5b4dfdd723773e955b62f35ccbaa7c7/Sources/GRPC/ConnectionManager.swift#L327

Seems ok to handle the errors via existing event-loop mechanisms instead of forcing them at the creation call site. Are there other considerations?

Lukasa commented 1 year ago

Are there any differences between the two?

Yes, substantial. ClientConnection is what it sounds like: a single connection. If that connection fails or hits errors, the connection is broken and all queued or in-flight RPCs fail.

GRPCChannelPool abstracts over the notion of having multiple connections, enabling better resilience.

Does GRPCChannelPool has some disadvantages?

It's a little more complex internally. That's about it.