adafruit / Adafruit_CircuitPython_Requests

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

Move socket specific code from `requests.Session` to `requests.SocketHandler` #146

Closed justmobilize closed 6 months ago

justmobilize commented 6 months ago

This comes from a thought and discussion to make sockets more usable and have fewer errors when connecting to multiple services (HTTP, MQTT, etc.).

I have come up with the following proposed steps:

  1. Move socket specific code from requests.Session to requests.SocketHandler
    • This is the Draft PR
  2. Make requests.SocketHandler a singleton
  3. Add some methods for better key<>socket handling, and helpers like get_open_keys()
  4. Add tests for new methods and changes
  5. Make sure Docs look good
  6. At this point mark this PR as Ready for review
  7. Update adafruit_esp32spi_socket.py to adafruit_esp32spi_socketpool.py
    • Since adafruit_esp32spi_socket.py returns a fake Socketpool it would be easier to follow code with more correct naming
  8. Update Adafruit_CircuitPython_MiniMQTT to take SocketHandler in __init__ and use it
  9. Add something like SocketHandler.lock_socket()
    • Which would make sure that you could always open a socket for a particular key: for example ("io.adafruit.com", 1883, "mqtt:") so you can always connect to AdafruitIO
  10. More update thoughts to come

As a side-note, I would love to put SocketHandler in it's own library, but feel that would break so many examples and things people have made...

justmobilize commented 6 months ago

Here is the current coverage report:

image
justmobilize commented 6 months ago

I am also happy to tackle https://github.com/adafruit/Adafruit_CircuitPython_Requests/issues/128 and move the handling of _FakeSSLSocket into SocketHandler for when is_ssl is True and self._ssl_context is None

justmobilize commented 6 months ago

Tested with a M4 Feather Express + AirLift FeatherWing using:

import adafruit_esp32spi.adafruit_esp32spi_socket as pool

esp = adafruit_esp32spi.ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset)
ssl_context = adafruit_requests.create_fake_ssl_context(pool, esp)

socket_handler = adafruit_requests.SocketHandler(pool, ssl_context)
requests = adafruit_requests.Session(pool, ssl_context, socket_handler)

And a ESP32-S2 via MagTag

pool = socketpool.SocketPool(wifi.radio)
ssl_context = ssl.create_default_context()

socket_handler = adafruit_requests.SocketHandler(pool, ssl_context)
requests = adafruit_requests.Session(pool, ssl_context, socket_handler)
jepler commented 6 months ago

I don't know for sure if anyone's doing this, but what if two ssl contexts are needed? It seems possible that you could need two ssl contexts, such as one with the normal set of certificates, and one with a special certificate needed for a single host that has a non-standard certificate.

Currently, this could be handled by two different Session objects. Looking at how the SocketHandler is used in the examples above, I see that the ssl_context is repeated in two places so I'm not sure how that would go.

justmobilize commented 6 months ago

That is True. Either you could create 2 different SocketHandlers (if they stay not a singleton), or the public get_socket_handler() could take in (pool, ssl_context) and return the once already created for that pair.

I think you would run into the same issue as current with 2 sessions and the fact they would stomp on top of each other and you would run out of sockets quickly.

justmobilize commented 6 months ago

Following up, the Session can totally keep track of the ssl_context, and only in the case it's not passed in would it generate one...

So from:

socket = self._socket_handler.get_socket(host, port, proto, timeout=timeout)

to:

socket = self._socket_handler.get_socket(host, port, proto, timeout=timeout, ssl_context=self._ssl_context)

This way, SocketHandler can safely be a singleton and not need to worry about handling multiple contexts