canvasapi
canvasapi copied to clipboard
#629 Implement default request timeouts
Proposed Changes
Adds a default API request timeout. I originally wanted to have a timeout keyword argument for every Requester.request call, but there were far too many instances of self._requester.request in the codebase, so I've instead insisted on a global default timeout.
I was unsure how to write the unit test for the mocked request wait and raise a ReadTimeout, so I'll just have to leave that up to a reviewer.
Fixes #629 .
A couple of preliminary thoughts:
- I like the idea of having a timeout, but the nuance of timeouts in requests may trip people up. According to the docs, providing an
int
will apply to theread
andwrite
timeouts. Would atuple
make more sense so the user explicitly has to set both values? - Related, this is not an upper limit on download time, just time to first bytes received. Does that matter?
- Do we need to subclass
CanvasException
to handle the Timeout exception emitted byrequests
? - A unit test with a sleep function greater than the timeout set on
Canvas
would be fine to make sure the error is captured correctly.
Hello @bennettscience, thanks for your thoughts. I'll commit and address your insights once I get feedback on my response:
- Sure, as it's consistent with the requests module.
- No, for the sake of consistency. Considering how widely used the requests module is for example with other API wrappers, having a timeout functionality that has consistent behavior is easier to understand. On second thought, maybe it's better to rename default_timeout to just timeout to emphasize that its interface mirrors the requests module's.
- Definitely. I didn't think about that initially. Though I would prefer if it instead subclassed ReadTimeout for safety, as that's what's explicitly thrown when the request timeout exceeds.
- Agreed. Would it work if I add a
exc=None
kwarg toregister_uris
and pass that intorequests_mocker.register_uri
soReadTimeout
s can be caught in awith self.assertRaises(ReadTimeout):
block?