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

requests-unixsocket should use the same timeout interface as requests. #44

Open
tomprince opened this issue Dec 23, 2019 · 3 comments · May be fixed by #62
Open

requests-unixsocket should use the same timeout interface as requests. #44

tomprince opened this issue Dec 23, 2019 · 3 comments · May be fixed by #62

Comments

@tomprince
Copy link

It seems like requests-unixsocket currently has its own way of setting timeouts, which differs from requests itself.

  • The default for requests-unixsocket is 60s unlike request's default of no timeout.
  • The timeout is set as a constructor parameter of UnixAdapter, which is inaccessible, if one uses requests_unixsocket.Session.
@TomGoBravo
Copy link

I think your second issue may be satisfied with https://github.com/msabramo/requests-unixsocket/pull/42/files (not released yet) and something like:

   session = requests_unixsocket.Session()
   adapter = requests_unixsocket.UnixAdapter(**your_kwargs_here)
   session.mount('http+unix://', adapter)

@msabramo
Copy link
Owner

https://pypi.org/project/requests-unixsocket/0.3.0/ now has #42 in case that helps.

msabramo added a commit that referenced this issue Dec 27, 2021
BREAKING CHANGE: Remove timeout code and `timeout` parameter from method
signatures, because it gives the impression of doing something, but
seems to be ineffective (see
#44).
@msabramo msabramo linked a pull request Dec 27, 2021 that will close this issue
@msabramo
Copy link
Owner

It looks to me like the timeout code I have isn't actually taking effect, maybe because requests is overriding it.

I made this small change to add some debugging:

diff --git a/requests_unixsocket/adapters.py b/requests_unixsocket/adapters.py
index 83e1400..42ef5d6 100644
--- a/requests_unixsocket/adapters.py
+++ b/requests_unixsocket/adapters.py
@@ -28,6 +28,7 @@ class UnixHTTPConnection(httplib.HTTPConnection, object):
         super(UnixHTTPConnection, self).__init__('localhost', timeout=timeout)
         self.unix_socket_url = unix_socket_url
         self.timeout = timeout
+        print(f'UnixHTTPConnection.__init__: self.timeout = {self.timeout}')
         self.sock = None

     def __del__(self):  # base class does not have d'tor
@@ -36,6 +37,7 @@ class UnixHTTPConnection(httplib.HTTPConnection, object):

     def connect(self):
         sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+        print(f'UnixHTTPConnection.connect: self.timeout = {self.timeout}')
         sock.settimeout(self.timeout)
         socket_path = unquote(urlparse(self.unix_socket_url).netloc)
         sock.connect(socket_path)

and then I do this:

$ .tox/py39/bin/ipython
Python 3.9.9 (main, Dec 23 2021, 15:32:01)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.30.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import json
   ...: import time
   ...:
   ...: import requests_unixsocket
   ...:
   ...: session = requests_unixsocket.Session()
   ...:
   ...: res = session.get('http+unix://%2FUsers%2Fabramowi%2Fdev%2Fgit-repos%2Frequests-unixsocket%2Fuds_socket/info')
UnixHTTPConnection.__init__: self.timeout = 60
UnixHTTPConnection.connect: self.timeout = None

Notice how my code sets self.timeout to 60 in the UnixHTTPConnection constructor, but by the time it gets to the connect method, where it calls sock.settimeout, self.timeout has been set to None.

The other thing that I'm noticing is that if I specify a timeout to session.get, then it seems to get applied correctly:

$ .tox/py39/bin/ipython
Python 3.9.9 (main, Dec 23 2021, 15:32:01)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.30.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import json
   ...: import time
   ...:
   ...: import requests_unixsocket
   ...:
   ...: session = requests_unixsocket.Session()
   ...:
   ...: res = session.get('http+unix://%2FUsers%2Fabramowi%2Fdev%2Fgit-repos%2Frequests-unixsocket%2Fuds_socket/info', timeout=3)
UnixHTTPConnection.__init__: self.timeout = 60
UnixHTTPConnection.connect: self.timeout = 3
---------------------------------------------------------------------------
...
ReadTimeout: UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=3)

So it seems to me that despite the incorrect-looking code that I have, the right thing seems to be happening (purely by accident 😄) and therefore I am inclined to simply remove my wrong-looking but ineffective and confusing timeout code and do a new release where I mark this as a breaking change, since I'd be removing the timeout parameter from the constructors for UnixHTTPConnection, UnixHTTPConnectionPool, and UnixAdapter.

See #62

msabramo added a commit that referenced this issue Dec 27, 2021
BREAKING CHANGE: Remove timeout code and `timeout` parameter from method
signatures, because it gives the impression of doing something, but
seems to be ineffective (see
#44 (comment)).
msabramo added a commit that referenced this issue Dec 28, 2021
BREAKING CHANGE: Remove timeout code and `timeout` parameter from method
signatures, because it gives the impression of doing something, but
seems to be ineffective (see
#44 (comment)).

Sem-Ver: api-break
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants