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.

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

Handle timeout and misc. errors #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle timeout and misc. errors #19

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
33 changes: 20 additions & 13 deletions amt/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,19 +81,26 @@ def __init__(self, address, password, username='admin', protocol='http'):
self.password = password

def post(self, payload, ns=None):
resp = requests.post(self.uri,
headers={'content-type':
'application/soap+xml;charset=UTF-8'},
auth=HTTPDigestAuth(self.username, self.password),
data=payload)
resp.raise_for_status()
if ns:
rv = _return_value(resp.content, ns)
if rv == 0:
return 0
print(pp_xml(resp.content))
else:
return 0
try:
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather not do the try here. If this is consumed as a library, it makes sense for these exceptions to flow up to the next layer so that they can handle these errors however they like.

Instead I would put the exception handling and error messages printed to the user here -

amt/bin/amtctrl

Line 115 in d3e5d74

except requests.exceptions.HTTPError as e:

Copy link
Contributor Author

@Rixillo Rixillo Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

OK, that looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another error I'd like to catch is "KeyboardInterrupt". If I control-C while looping calls to amtctrl, the stack trace dump makes a mess out of my terminal screen. Thanks.

^CTraceback (most recent call last):
  File "/usr/bin/amtctrl", line 5, in <module>
    pkg_resources.run_script('amt==0.6.0', 'amtctrl')
  File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 540, in run_script
    self.require(requires)[0].run_script(script_name, ns)
  File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 1455, in run_script
    execfile(script_filename, namespace, namespace)
  File "/usr/lib/python2.7/site-packages/amt-0.6.0-py2.7.egg/EGG-INFO/scripts/amtctrl", line 119, in <module>
    sys.exit(main())
  File "/usr/lib/python2.7/site-packages/amt-0.6.0-py2.7.egg/EGG-INFO/scripts/amtctrl", line 105, in main
    print(amt.wsman.friendly_power_state(client.power_status()))
  File "/usr/lib/python2.7/site-packages/amt-0.6.0-py2.7.egg/amt/client.py", line 144, in power_status
    data=payload)
  File "/usr/lib/python2.7/site-packages/requests/api.py", line 108, in post
    return request('post', url, data=data, json=json, **kwargs)
  File "/usr/lib/python2.7/site-packages/requests/api.py", line 50, in request
    response = session.request(method=method, url=url, **kwargs)
  File "/usr/lib/python2.7/site-packages/requests/sessions.py", line 464, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/lib/python2.7/site-packages/requests/sessions.py", line 582, in send
    r = dispatch_hook('response', hooks, r, **kwargs)
  File "/usr/lib/python2.7/site-packages/requests/hooks.py", line 41, in dispatch_hook
    _hook_data = hook(hook_data, **kwargs)
  File "/usr/lib/python2.7/site-packages/requests/auth.py", line 188, in handle_401
    _r = r.connection.send(prep, **kwargs)
  File "/usr/lib/python2.7/site-packages/requests/adapters.py", line 370, in send
    timeout=timeout
  File "/usr/lib/python2.7/site-packages/urllib3/connectionpool.py", line 544, in urlopen
    body=body, headers=headers)
  File "/usr/lib/python2.7/site-packages/urllib3/connectionpool.py", line 372, in _make_request
    httplib_response = conn.getresponse(buffering=True)
  File "/usr/lib64/python2.7/httplib.py", line 1089, in getresponse
    response.begin()
  File "/usr/lib64/python2.7/httplib.py", line 444, in begin
    version, status, reason = self._read_status()
  File "/usr/lib64/python2.7/httplib.py", line 400, in _read_status
    line = self.fp.readline(_MAXLINE + 1)
  File "/usr/lib64/python2.7/socket.py", line 476, in readline
    data = self._sock.recv(self._rbufsize)
KeyboardInterrupt

resp = requests.post(self.uri,
headers={'content-type':
'application/soap+xml;charset=UTF-8'},
auth=HTTPDigestAuth(self.username, self.password),
data=payload,
timeout=5.0)
Copy link
Owner

Choose a reason for hiding this comment

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

The request timeout is typically just a backstop, and I'd be concerned that 5 seconds might not be enough, How about we make it 30?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, 5 seconds is too short. It would be nice to make this a command line arg, because it can vary widely depending on your situation

resp.raise_for_status()
except requests.exceptions.Timeout:
print "Request timed out."
except requests.exceptions.RequestException as e:
print e
else:
if ns:
rv = _return_value(resp.content, ns)
if rv == 0:
return 0
print(pp_xml(resp.content))
else:
return 0

def power_on(self):
"""Power on the box."""
Expand Down