Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stopService() leaves pooled connections open #86

Closed
JayH5 opened this issue Nov 11, 2016 · 5 comments · Fixed by #140 or #156
Closed

stopService() leaves pooled connections open #86

JayH5 opened this issue Nov 11, 2016 · 5 comments · Fixed by #140 or #156
Labels
Milestone

Comments

@JayH5
Copy link
Contributor

JayH5 commented Nov 11, 2016

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.

@mithrandi mithrandi added the bug label Nov 11, 2016
@mithrandi
Copy link
Contributor

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.

@mithrandi
Copy link
Contributor

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.

@adiroiban
Copy link
Member

But why AcmeIssuingService uses multiple clients?

Why not create a single client instance in startService() and stop it in stopService()?

@adiroiban
Copy link
Member

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().

mithrandi added a commit that referenced this issue Feb 14, 2020
[Fixes #86] Close the HTTP pool when service is stopped.
glyph added a commit that referenced this issue Feb 24, 2020
Revert "[Fixes #86] Close the HTTP pool when service is stopped."
@glyph
Copy link
Member

glyph commented Aug 11, 2020

The merge that fixed this was reverted so this should have been reopened. Whoops!

@glyph glyph reopened this Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants