adafruit / Adafruit_CircuitPython_Requests

Requests-like interface for web interfacing
MIT License
51 stars 36 forks source link

iface is not set correctly when using some examples #48

Closed dhalbert closed 3 years ago

dhalbert commented 3 years ago

I'm not sure whether this is an error in the examples or in the revised legacy API.

examples/requests_simpletest.py and examples/requests_advanced.py both contain code:

# Initialize a requests object with a socket and esp32spi interface
socket.set_interface(esp)
requests.set_socket(socket)

However, this code stopped working at some point during the revision of the legacy API. If I try the simpletest, I get

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "requests_simpletest.py", line 51, in <module>
  File "adafruit_requests.py", line 636, in get
  File "adafruit_requests.py", line 535, in request
  File "adafruit_requests.py", line 420, in _get_socket
  File "adafruit_requests.py", line 604, in wrap_socket
AttributeError: 'NoneType' object has no attribute 'TLS_MODE'

Here is the relevant revised code:

class _FakeSSLContext:
    def __init__(self, iface):
        self._iface = iface

    def wrap_socket(self, socket, server_hostname=None):
        """Return the same socket"""
        # pylint: disable=unused-argument
        return _FakeSSLSocket(socket, self._iface.TLS_MODE)

def set_socket(sock, iface=None):
    """Legacy API for setting the socket and network interface. Use a `Session` instead."""
    global _default_session  # pylint: disable=global-statement,invalid-name
    _default_session = Session(sock, _FakeSSLContext(iface))
    if iface:
        sock.set_interface(iface)

The _FakeSSLContext ends up with a None iface, and then wrap_socket() fails because self._iface.TLS_MODE will fail.

makermelissa commented 3 years ago

If you use requests.set_socket(socket, esp) like the PyPortal library does, it should work correctly. Maybe the examples just need to be updated?

dhalbert commented 3 years ago

I think that's true, but maybe the examples used to work (by accident??) The old code is this:

class _FakeSSLContext:
    @staticmethod
    def wrap_socket(socket, server_hostname=None):
        """Return the same socket"""
        # pylint: disable=unused-argument
        return socket

def set_socket(sock, iface=None):
    """Legacy API for setting the socket and network interface. Use a `Session` instead."""
    global _default_session # pylint: disable=global-statement,invalid-name
    _default_session = Session(sock, _FakeSSLContext())
    if iface:
        sock.set_interface(iface)

wrap_socket() doesn't use the iface, so it won't have an error.

dhalbert commented 3 years ago

Is the socket.set_interface() just unnecessary if you do requests.set_socket(socket, esp)? I am just not familiar with the idioms for using the library.

makermelissa commented 3 years ago

Hmm, perplexing. 🤔

dhalbert commented 3 years ago

It does work to replace the two lines with the one. (The point of all this was just to test that the revised requests library still worked on AirLift with SSL.)

anecdata commented 3 years ago

Here's what I do:, and it works in CircuitPython. There may be a different sequence to be fully compatible with CPython:

# SP32-S2:

    pool = socketpool.SocketPool(wifi.radio)
    requests = adafruit_requests.Session(pool, ssl.create_default_context())

# ESP32SPI:

    # Requests (legacy API)
    requests.set_socket(socket, esp)

I don't know of a single code sequence that works in CircuitPython on both non-ESP32-S2 and ESP32-S2. The Requests import is typically different between the two also. I suspect Scott is aiming for CPython compatibility on all platforms as he reworks the legacy stack.

tannewt commented 3 years ago

@dhalbert I think the simplest thing is to have set_socket pull the interface from the socket if it isn't provided. It's all a hack for backwards compatibility so we can do sketchy things in it.