Skip to content

Variable Request Timeouts #49

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

Closed
anecdata opened this issue May 29, 2019 · 4 comments
Closed

Variable Request Timeouts #49

anecdata opened this issue May 29, 2019 · 4 comments

Comments

@anecdata
Copy link
Member

anecdata commented May 29, 2019

adafruit_esp32spi_requests.py request() sets socks.settimeout() to 1 second. This is fine in many cases, but due to server, traffic, and various other issues, it may not be enough to reliably fetch data. Retries can partially resolve this, but more user control over timeouts would give users the ability to make their own trade-offs between speed and reliability.

I'd propose initially making the timeout within request() into a keyword argument with default of 1 second, and rippling it up to WiFiManager and device-specific classes like PyPortal.

Other than ping ttl and hard-coded delays and timeouts deep in ESP_SPIcontrol, everything in ESP32SPI appears to rely on the 1-second timeout set in the request (notably in socket read and readline). Other than ping and wifi scans, NINA firmware doesn't seem to introduce any substantial delays or timeouts that I could find. I'm not sure what the ESP core does. I haven't tested to determine if there's a max name lookup time within the system (DNS servers will eventually time out and provide an error response).

Ideally (long-term), perhaps users could have separate control over timeouts for name lookup, connections, and requests. In theory, CircuitPython users would typically be fetching more concise payloads than a typical web user, but for reference, browsers have many-second timeouts for connections and requests. cURL also has separate connection and overall timeouts, with relatively high defaults (300 on connection, ∞ on the overall request). We don't need to be concerned about persistent connections for now since ESP32SPI uses HTTP/1.0 and there is no keep-alive.

@anecdata
Copy link
Member Author

anecdata commented May 31, 2019

I can do a PR. I would like someone who is more pythonically experienced and has a good sense of the architecture to confirm whether this is a reasonable approach, or suggest an alternative.

I'll run this change on a bunch of devices all weekend. Experience so far is much better reliability with programmatic control of timeout.

Another reference point: @kenneth-reitz's @requests library supports connection timeouts (default=∞) and read timeouts, (or a single value to be used for both), and docs note that:

It’s a good practice to set connect timeouts to slightly larger than a multiple of 3, which is the default TCP packet retransmission window.
https://2.python-requests.org/en/master/user/advanced/#timeouts

@brentru
Copy link
Member

brentru commented May 31, 2019

@anecdata when you submit your PR, tag the CircuitPython Librarians group for a code review.

@makermelissa
Copy link
Collaborator

It sounds like a reasonable approach to me. :)

@anecdata
Copy link
Member Author

OK, thanks. I'll test for a while longer, then submit.

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

No branches or pull requests

3 participants