Skip to content

Timeout added for opener #42

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Timeout added for opener #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions python_http_client/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,18 +143,20 @@ def _build_client(self, name=None):
url_path=url_path,
append_slash=self.append_slash)

def _make_request(self, opener, request):
def _make_request(self, opener, request, timeout=4):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems incomplete as I don't see a way to get a timeout value down into this call from the public api. It looks to me like the Client.getattr method's nested http_request should take a timeout parameter from the **kwargs and then pass that on to the call to self._make_request. You're hard coding a 4 second timeout here which isn't right - that should be up to the caller. The default timeout shouldn't be set to anything either to stay consistent with how every other api I've used handles timeouts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feedback @wynnw!

@prabhakar267 thoughts?

"""Make the API call and return the response. This is separated into
it's own function, so we can mock it easily for testing.

:param opener:
:type opener:
:param request: url payload to request
:type request: urllib.Request object
:param timeout: timeout for request in seconds, default 4 sec
:type integer:
:return: urllib response
"""
try:
return opener.open(request)
return opener.open(request, timeout=timeout)
except HTTPError as err:
exc = handle_error(err)
exc.__cause__ = None
Expand Down Expand Up @@ -224,7 +226,7 @@ def http_request(*_, **kwargs):
if data and not ('Content-Type' in self.request_headers):
request.add_header('Content-Type', 'application/json')
request.get_method = lambda: method
return Response(self._make_request(opener, request))
return Response(self._make_request(opener, request, timeout=4))
return http_request
else:
# Add a segment to the URL
Expand Down
2 changes: 1 addition & 1 deletion tests/test_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def __init__(self, host, response_code):
self.response_code = 200
Client.__init__(self, host)

def _make_request(self, opener, request):
def _make_request(self, opener, request, timeout):
if 200 <= self.response_code <299: # if successsful code
return MockResponse(self.response_code)
else:
Expand Down