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

Handle timeout and misc. errors #19

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

Conversation

Rixillo
Copy link
Contributor

@Rixillo Rixillo commented Mar 31, 2017

Addresses issue #17


This change is Reviewable

At first attemp, I put the file in the wrong directory.
Copy link
Owner

@sdague sdague left a comment

Choose a reason for hiding this comment

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

There are good things to catch, lets do them in a different place in the code though.

'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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants