bravado icon indicating copy to clipboard operation
bravado copied to clipboard

use threading.TIMEOUT_MAX for timeout

Open tschaume opened this issue 3 years ago • 4 comments

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 avatar Jan 26 '21 21:01 tschaume

@analogue any plans to consider this PR for merging?

tschaume avatar Mar 18 '21 22:03 tschaume

@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?

tschaume avatar Apr 15 '21 22:04 tschaume

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)

macisamuele avatar Apr 18 '21 21:04 macisamuele

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!

mkhorton avatar Apr 30 '21 19:04 mkhorton