Memory leak in @protobuf-ts/grpc-transport

[dddenis]

@grpc/grpc-js Client is being created on every call and never disposed after.

Demo of the issue can be found here
Example of 2 clients running 1000 unary calls and reporting memory usage after:

1. make in packages/example-node-grpc-server/ to start the server
2. make in packages/example-node-grpc-client/ to see correct memory usage
3. make in packages/example-node-grpc-transport-client/ to see clients not being garbage collected

The solution would be to either close the client after request, or create it once in GrpcTransport constructor and then reuse.

[timostamm]

Great feedback, thanks.

Closing the client after each request plugs the memory leak, but I think this doesn't go in the right direction.

I am pretty sure that @grpc/grpc-js will keep connections alive (to save expensive TLS handshakes) until you close a channel (by client.close()). At least that is what I would expect from a gRPC client.

So the best way forward seems to be to create the client once in the transport (fixing the memory leak, allowing connection re-use) and expose client.close() with a close() method on the transport.

There is a minor problem with that: We accept host, channelCredentials and clientOptions for each individual call. This only works because we create a new client each time. This should be refactored, probably by splitting the GrpcOptions type into one type for transport options and one type for call options.

[frederikhors]

@timostamm since I cannot ask you on ETA for this, can I ask you if there is a temporary workaround?

In my SPA (single-page-app) I'm not closing/destroying services once opened (with new PlayerServiceClient(transport)).

So I think I'm in full memory leak, right? (I don't know how to measure it.)

[ghost]

@frederikhors No, you are not affected. This is about the gRPC implementation, but your SPA uses gRPC-web.

[dddenis]

@timostamm I agree that the solution with creating @grpc/grpc-js Client once and reusing connection is the right one.

I've submitted a PR with partial implementation of this approach (without exposing close method on the protobuf-ts Client).
It should solve the memory leak issue, and allow the user to close GrpcTransport directly, therefore disposing of @grpc/grpc-js Client underneath.

I've also rebased the demo branch on top of the fix:

master:

{
  startHeap: 78.52119445800781,
  finishHeap: 85.61224365234375,
  diffHeap: 7.0910491943359375
}

fix:

{
  startHeap: 79.07605743408203,
  finishHeap: 79.87208557128906,
  diffHeap: 0.7960281372070312
}

let me know if there are any changes you would like me to make.