stopService() leaves pooled connections open
JayH5 opened this issue · 5 comments
With the default client, a persistent HTTPConnectionPool is created for connections to the ACME directory. This pool isn't tracked, however, and when stopService() is called on the txacme service a pooled connection remains waiting on the reactor. This breaks my integration tests.
In the txacme integration tests, I do this terrible thing:
def _create_client(self, key):
return (
Client.from_url(reactor, LETSENCRYPT_STAGING_DIRECTORY, key=key)
.addCallback(tap(
lambda client: self.addCleanup(
client._client._treq._agent._pool.closeCachedConnections)))
)
You could do something similar in the client creator you pass to AcmeIssuingService, but obviously this is an awful hack. A better approach would probably be to pass the jws_client argument which would allow creating (and subsequently cleaning up) the connection pool under control of your tests.
Unfortunately the reason the service can't do this for you is exactly because client creation is indirected this way; the service can't know how your client is actually put together, so it can't reach into it to shut down a connection pool.
Having said that, I realised that the way client creation works now (it changed after I originally wrote that code), connections from the connection pool will never be reused; so maybe we should create the default client with HTTPConnectionPool(persistent=False), thus never leaving any connections around to clean up. I'll leave this issue open for purposes of making that change.
Oh, that's not quite right; a new client is created for each issuing run, but within that run the pool will be able to reuse the same connection for subsequent requests. Hmm. I'll think about this some more, there should be something better we can do; maybe add some kind of cleanup hook to the client object.
But why AcmeIssuingService uses multiple clients?
Why not create a single client instance in startService() and stop it in stopService()?
As a workaround, I am creating the client ouside of client_creator and in client_creator I am just returning the same client instance.
In this way, there is a single client in use and I can call client.stop() before calling AcmeIssuingService.stopService().
The merge that fixed this was reverted so this should have been reopened. Whoops!