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

use threading.TIMEOUT_MAX for timeout #472

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

Conversation

tschaume
Copy link

Using timeout=None as default causes the OverflowError to be triggered in Windows. This PR fixes #470 by using threading.TIMEOUT_MAX (docs) as default. It also relates to the discussion in Yelp/fido#52.

@tschaume
Copy link
Author

@analogue any plans to consider this PR for merging?

@tschaume
Copy link
Author

@macisamuele I noticed you reviewed another recent PR :) Any chance you could look over this one, too? Do you also have merge and PyPI release permissions for this repo?

@macisamuele
Copy link
Collaborator

As I'm no longer a Yelp employee and as I'm getting less exposure to the library usage I would defer the decision to @analogue (or someone currently within the Yelp org).


My 2 cents around the PR

  • A change of the default timeout (from None to TIMEOUT_MAX) does represent a backward incompatible change and as such it would require a major version bump.
  • I'm not fully sure I do understand what it is trying to fix and more important if bravado is actually the best place to fix it (seems an issue within the fido handling of "infinite" timeout).
  • If we were to agree that the default value should be moved from None to TIMEOUT_MAX do we still want to accept None as a possible timeout value? If not then we would need to fix the typing annotations associated to timeout.

My recommendation in this case would be to expand more on

  • describing what problem you are trying to fix (from the linked issue I cannot reproduce it)
  • how this PR fixes the problem (eventually explaining why this is the best place to do so)

@mkhorton
Copy link

Hi, just to add that I've also seen reports of multiple Windows users encountering this bug who are using our bravado-powered API client (specifically, they encounter a OverflowError: timeout value is too large). A merge or a fix would be much appreciated!

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.

OverflowError: timeout value is too large
3 participants