Closing of finagle-thrift client with scrooge generated service

[felixbr]

I'm looking for a way to close a thrift-client, which connects to a service (generated via scrooge).

Expected behavior

If I generate a thrift-client like this

Thrift.client.build[EchoService.MethodPerEndpoint]("host:port")

I get back a client which has a asClosable method and calling close() on it should shutdown the client-connections, thread-pools, and other resources.

Actual behavior

The close() method doesn't seem to do anything becauseasClosable is always generated as Closable.nop:

def asClosable: _root_.com.twitter.util.Closable = _root_.com.twitter.util.Closable.nop

Steps to reproduce the behavior

I used sbt-scrooge to generate a service with this thrift-file:

service EchoService {
  string echo(1: required string input)
}

Then I tried to start and close a service with that:

val client = Thrift.client.build[EchoService.MethodPerEndpoint]("host:port")

Await.ready(client.asClosable.close())

I'm aware that in this snipped you cannot really observe whether the underlying resources are still not closed, so if you want, I can try to write a project to demonstrate it. That said, it's fairly easy to see if you look at the generated code in EchoService.

Maybe there's a scrooge config option I don't know about or I need to do something else to close a generated thrift-client. In that case I'd be happy to learn about it.

Cheers ~ Felix

[jyanJing]

Thanks @felixbr for reaching out!

The thrift-client should override the asClosable so it shouldn't be nop.

To verify if the underlying resources is closed or not, you could follow this example to write a similar test.

Please let me know how it goes!

[felixbr]

Thanks for the tip, @jyanJing!

I wrote a test which verifies closing via the ServiceClosedException as one of the tests you linked does. Seems to work well: https://github.com/felixbr/finagle-effect/commit/6b4c9bd50b47de7694e3664b2fd684ad9e532b14

I haven't checked if closing it also cleans up connections and thread pools but I trust that this is already well-tested 🙂

Thanks again and have a nice weekend!