Enable with statement #185
Conversation
By implementing those two functions, the BaseAPI object can be used with
a 'with' statement like this:
with pypuppetdb.connect() as session:
# ...
This will ensure that all HTTP connections are explicitly closed when
exiting the "with" context, thus avoiding possible hangs because of the
open connects.
As stated in the comment, this can happen if our HTTP connections are
sent through a tunnel (for example an ssh tunnel) which will then wait
for connections to close before closing the tunnel.
Explicitly closing HTTP connections will indicate to any tunnel that
we're done with our work and that the tunnel can also be closed in turn.
|
|
||
| def test_connect_with_statement(): | ||
| with pypuppetdb.connect() as puppetdb: | ||
| assert puppetdb.version == 'v4' |
There was a problem hiding this comment.
can we also assert that the connection is closed?
There was a problem hiding this comment.
that's a pretty pertient request! I was wondering the same when writing the test and ended up with the interrogation "how can I check this with the current tooling?"
our best bet, I think, would be to mock out either the disconnection method or the requests.Session.close() method and verify that the mock is getting called.
related to this yesterday I was thinking that for folks who don't want to/can't use the "with" statement, should we also provide a .disconnect() method so that it's possible to call it manually? If we stub out requests.Session.close() we can test that disconnection happens as expected in the non-"with" test case, too.
(I'm just not exactly sure how to use the mock and verify that the mocked out function is getting called... but I guess I can search around in the library's documentation)
When we exit the "with" block used on the connect() function, we expect HTTP sessions to be closed, so we should make sure that it is really happening.
This new method can help ppl tearing things down without using the "with" statement. Similarly to the motivation behind implmenting the "with" statement, this can be useful for connections that were made through a tunnel (for example ssh). This method makes it possible for example to have a BaseAPI object wrapped up in another class, and tear down connections only when that wrapping class is all done with calls to the object.
This is similar to the test we are doing in the case of the "with" statement, but we're making sure that calling .disconnect() directly causes the same effect.
|
I've used "import mock" in the file
|
We should definitely be migrating to pytest, but that might be too much for this pr, or? |
I feel this way, too! It just felt important to mention why I chose the mocking library that I did and wanted to throw the migration ideas to assess a future direction. But by all means, if we can avoid migrating to another mocking library in this PR, it would keep this patch set easier to review. |
WIth this change it becomes possible to enclose puppetdb calls inside a "with" block so that all HTTP connections get explicitely closed when we're done.
I've had to explicitly close connections when working with paramiko for tunneling puppetdb requests through ssh.