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

#629 Implement default request timeouts #630

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ArhanChaudhary
Copy link

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 .

@bennettscience
Copy link
Contributor

bennettscience commented Jun 15, 2023

A couple of preliminary thoughts:

  1. 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 the read and write timeouts. Would a tuple make more sense so the user explicitly has to set both values?
  2. Related, this is not an upper limit on download time, just time to first bytes received. Does that matter?
  3. Do we need to subclass CanvasException to handle the Timeout exception emitted by requests?
  4. 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.

@ArhanChaudhary
Copy link
Author

Hello @bennettscience, thanks for your thoughts. I'll commit and address your insights once I get feedback on my response:

  1. Sure, as it's consistent with the requests module.
  2. 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.
  3. 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.
  4. Agreed. Would it work if I add a exc=None kwarg to register_uris and pass that into requests_mocker.register_uri so ReadTimeouts can be caught in a with self.assertRaises(ReadTimeout): block?

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4b4fbd8) to head (3819739).

Additional details and impacted files
@@            Coverage Diff            @@
##           develop      #630   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        73           
  Lines         3740      3741    +1     
=========================================
+ Hits          3740      3741    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Request timeouts
3 participants