Terrance
commented
1 year ago From the example you've given, you appear to be creating a Skype instance within a HTTP request hander, which is not an intended use case -- you should have a single instance which persists across the lifetime of your application. The example itself also uses an asynchronous context when SkPy is not async-supporting library, which sets a bad precedent of using synchronous code within an async block without using an executor.
Main code should also be further improved to handle closure of session
It would perhaps be useful to provide a Skype.close() method (which calls a SkypeConnection.close() method that just closes the session, or indeed shaping that as an __enter__/__exit__ pair) so that the user does not need to be aware of SkPy's use of a Requests session, and I guess technically that's not a breaking change as the existing behaviour of not closing the session remains unchanged.
As this PR stands, I'd want to see an improved example, but I don't currently see the need to include this warning. If it's not sufficiently obvious that Skype objects are intended to be long-life, that might be something to note in the docs.
This is to update documentation to inform users that they should close sessions if this is used as part of bigger project. Not doing so will leave bunch of CLOSE_WAIT connections - where connection is terminated from one side (probably microsoft) but not cleared on python side because even if this module was executed and finished, original process that the code was executed from is still running - hence not clearing any open sockets.
Not closing the session leads to bunch of piled up open sockets which will eventually exceed ulimit size for maximum open files. This could lead to service instability.
In SkypeConnection class we can see that the
requests.Session()object was created : https://github.com/Terrance/SkPy/blob/bfb7f657e7f6cc5147cf4f2e060f672e28aa64f1/skpy/conn.py#L152-L164However when looking at the other parts of code, it appears it's never closed. Sure, when you use this module as part of one-time run script - once the process is closed it will close all remaining open file descriptors and sockets, but if the main process is still running - that will not happen.
Main code should also be further improved to handle closure of session, so there would be no need from user to close it manually. I don't have time to dedicate to that now, without potentially making feature breaking changes (for example Skype or SkypeConnection class to be used as context manager) - but it is something to think about.