shareplum
shareplum copied to clipboard
jasonrollins
Not closing sessions.
During my unit testing I got a warning that a socket was left open.
ResourceWarning: unclosed <socket.socket fd=9, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('10.25.207.70', 35219), raddr=('10.22.230.61', 911)>
For now, I'm doing...
class Site(SuperSite):
""" Adding enter/exit methods to Site. """
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
def __enter__(self):
return self
def __exit__(self, exc_type, exc_value, exc_traceback):
self._session.close()
And calling using:
with Site(site_url, auth=auth, verify_ssl='/etc/ssl/certs/ca-certificates.crt') as site:
Please add some functionality to close connections--possibly implementing __enter__ and __exit__ as I have above.
Thanks!
No, I'm only using super here to subclass your Site class. Here SuperSite is just a renamed import.
from shareplum import Site as SuperSite
I believe you should only need the functions
def __enter__(self):
return self
def __exit__(self, exc_type, exc_value, exc_traceback):
self._session.close()
added to you Site class. That's basically what I'm doing in the above, and it's working well for me. : )
I wasn't able to get this to work, but you can use this in unittest to resolve the error:
def tearDown(self): self.site._session.close()
Do you still think SharePlum needs to implement "with"?
Is this solution really addressing the right problem? I'm guessing the test framework detecting a socket left open is down to a connection being kept alive where it could maybe be closed. I don't think the session itself needs to be closed.
If we wanted to avoid sessions being kept alive we could add {'Connection':'close'} to the headers or I think you can specify it in the config of the session. I'm not sure if we want to do this or not though. I guess closing the session in the way you do in your PR doesn't do any harm. I feel like the test warning isn't really something to worry about in practice though.
There currently is not any way to close the session except directly accessing the session. Just seems like a bad interface to me. I am not okay not closing my sessions/sockets in production, and I don't know (my own knowledge limitation, I may do research) when the session would be closed if adding {'Connection':'close'} to the header.
The context manager methods aenter/aexit are as you mention harmless to add, and make closing of the session easy and natural.
Like I said, I do think it would be harmless. However, I was pointing it it's not the session you were having a problem with but a connection. You can let the session get cleaned up, when it goes out of scope, which will happen some time after the site goes out of scope.
Have you found any best practice or session documentation that says session.close should be called? I'd be interested to read it.