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

Add HTTP1.1 support and socket reuse #31

Merged
merged 20 commits into from
Sep 14, 2020
Merged

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Sep 2, 2020

Reusing sockets makes sharing memory easier and keeping the connection open will make them faster.

This PR also moves to a model of passing the socket and ssl implementation into Session instead of into the module with a function.

json parsing will now happen character-by-character on CircuitPython and lower the peak memory used and the largest allocation size.

It includes legacy compatibility so it shouldn't break anything. Corresponding libraries will benefit more from this when they implement recv_into so they don't allocate a new buffer every read.

This is draft because I need to:

  • Lint it all.

@tannewt tannewt requested a review from brentru September 2, 2020 20:00
@tannewt tannewt linked an issue Sep 3, 2020 that may be closed by this pull request
@tannewt tannewt marked this pull request as ready for review September 3, 2020 00:49
@FoamyGuy
Copy link
Contributor

FoamyGuy commented Sep 5, 2020

Does this need a new version of adafruit_esp32spi? I tried this on a PyPortal with a basic GET request example but I am consistently getting this exception:

Fetching text from http://wifitest.adafruit.com/testwifi/index.html
Traceback (most recent call last):
  File "code.py", line 40, in <module>
  File "/lib/adafruit_requests.py", line 566, in get
  File "/lib/adafruit_requests.py", line 454, in request
  File "/lib/adafruit_requests.py", line 403, in _get_socket
  File "/lib/adafruit_requests.py", line 399, in _get_socket
  File "adafruit_esp32spi/adafruit_esp32spi_socket.py", line 75, in connect
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 750, in socket_connect
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 649, in socket_open
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 323, in _send_command_get_response
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 306, in _wait_response_cmd
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 293, in _wait_response_cmd
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 275, in _check_data
RuntimeError: Expected 01 but got 00

@tannewt
Copy link
Member Author

tannewt commented Sep 8, 2020

@FoamyGuy No, it's not fully backwards compatible atm. I'm hoping to get to that today. Specifically, the current iteration doesn't use an IP address for HTTP connections and doesn't set TLS_MODE during .connect. Both are non-standard behavior I missed by testing with CPython's socket.

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, read through it to get a better sense of how the socket pools will work in MiniMQTT. I have a few questions.

adafruit_requests.py Show resolved Hide resolved
adafruit_requests.py Outdated Show resolved Hide resolved
# "password" keys with your WiFi credentials. DO NOT share that file or commit it into Git or other
# source control.
# pylint: disable=no-name-in-module,wrong-import-order
from secrets import secrets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to add a more verbose try/except for users who do not have a secrets.py file on their FS: https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/examples/esp32spi_aio_post.py#L15

adafruit_requests.py Show resolved Hide resolved
@anecdata
Copy link
Member

anecdata commented Sep 11, 2020

With this version of Requests and:
Adafruit CircuitPython 6.0.0-alpha.3-142-g683462c1b on 2020-09-10; Saola 1 w/Wrover with ESP32S2
(S3 .bin corresponding to "Merge pull request #3326 from tannewt/native_wifi")
test code.py download from Discord yesterday

I consistently get:

  File "code.py", line 34, in <module>
  File "/lib/adafruit_requests.py", line 505, in get
  File "/lib/adafruit_requests.py", line 492, in request
  File "/lib/adafruit_requests.py", line 102, in __init__
RuntimeError: Unable to read HTTP response.

The older version (Discord version) of Requests generally works, some intermittent errors:
OSError: Failed SSL handshake in _get_socket,
espidf.MemoryError: in _get_socket,
TypeError: unsupported types for __sub__: 'NoneType', 'int' in _readinto (attributed to dev version of chunking code
OSError: [Errno 9] EBADF in _readto)

* Add TLS_MODE to old connect() calls
* Provide ip to connect when in HTTP mode
@tannewt
Copy link
Member Author

tannewt commented Sep 11, 2020

Ok, @brentru and @anecdata this should be good to go.

@anecdata
Copy link
Member

RuntimeError: Unable to read HTTP response.
fixed by
adafruit/circuitpython#3397
6.0.0-alpha.3-182-g0c7212250 on 2020-09-11

@anecdata
Copy link
Member

Some edge cases. (I'm tracking down why I still get RuntimeError: Unable to read HTTP response. under certain circumstances; the typical text/json websites we usually test with seem fine.)

Here's one: Zero-length response bodies should be valid, but try this:

for _ in range(0, 5):
    response = requests.get("https://httpbin.org/status/200")
    # print(response.status_code)
    # print(response.reason)
    # print(response.headers)
    # print(response.text)

    print()

yields some odd behavior, sometimes gets RuntimeError: Unable to read HTTP response. It waits for about a minute. There is a {'Content-Length': '0'} header.

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes resolved and looks good.

@tannewt
Copy link
Member Author

tannewt commented Sep 14, 2020

@anecdata Can you reproduce that with CPython sockets? Sounds like it may be a timeout thing.

@anecdata
Copy link
Member

anecdata commented Sep 14, 2020

import requests

for _ in range(0, 5):
    response = requests.get("https://httpbin.org/status/200")
    print(response.status_code)
    print(response.reason)
    print(response.headers)
    print(response.text)

That URL returns right away consistently in browser and python3 on macOS, with header:

{'Date': 'Mon, 14 Sep 2020 20:24:48 GMT', 'Content-Type': 'text/html; charset=utf-8', 'Content-Length': '0', 'Connection': 'keep-alive', 'Server': 'gunicorn/19.9.0', 'Access-Control-Allow-Origin': '*', 'Access-Control-Allow-Credentials': 'true'}

If there's another test procedure I can try, please point me in the right direction.

It doesn't always exception on CicruitPython, but typically at least once in 5 tries. It may be two separate issues, but with a content-length header, client should be able to return right away.

@anecdata
Copy link
Member

This may be a clue: if I manually shorten the requests timeout in CircuitPython: requests.get("https://httpbin.org/status/200", timeout=5), it comes back in 5 seconds rather than a minute, and I don't get the "Unable to read HTTP response" exception so far.

@anecdata
Copy link
Member

anecdata commented Sep 14, 2020

BTW, noticed this the other day and it seems consistent... if a scan isn't done first, wifi.radio.connect() does not return (I did a keyboard interrupt after a few minutes, and the trace indicated the wifi.radio.connect() lines).

oops, I guess that doesn't belong in this PR

@tannewt
Copy link
Member Author

tannewt commented Sep 14, 2020

@anecdata Ok, sounds like these are ESP32-S2 wifi related issues, not with this library itself.

@anecdata
Copy link
Member

Should I file... 2 issues on circuitpython?
• 'Content-Length': '0'
• scan before connect

@tannewt
Copy link
Member Author

tannewt commented Sep 14, 2020

@anecdata Yes please!

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.

Requests with CPython Native Socket
4 participants