Allow optional nbytes parameter for recv_into

[tekktrik]

That sweet, sweet hotfix for Issue #154! Sorry for breaking some things, and thanks to @FoamyGuy to summarizing. Default argument is None which still triggers filling buffer as much as possible, length should override that now.

[FoamyGuy]

I think we still have some kind of issue related to this. When I run the requests simpletest on a PyPortal with 7.2.0-alpha-1 I am consistently getting this error raised:

code.py output:
Connecting to AP...
Connected to AloeVera   RSSI: -42
Fetching text from http://wifitest.adafruit.com/testwifi/index.html
Traceback (most recent call last):
  File "code.py", line 58, in <module>
  File "/lib/adafruit_requests.py", line 847, in get
  File "/lib/adafruit_requests.py", line 719, in request
  File "/lib/adafruit_requests.py", line 216, in __init__
RuntimeError: Unable to read HTTP response.

Code done running.

I tried several times and it always returns the same error, has never completed the request successfully for me.

I looked into this a bit during my stream today but was unable to determine the real root cause (the details of the buffer handling and internals of requests and esp32spi socket a bit over my head ATM).

[FoamyGuy]

I think this while loop is evaluating to false and we aren't going inside of it. I added a debugging print above this to print the values of limit and to_read: print("to_read: {} - limit: {}".format(to_read, limit))

And I'm seeing these:

to_read: 32 - limit: 32

I also tried modifying the requests library to not pass the nbytes argument and it seemed to get stuck in an infinite loop, but did eventually finish after a few minutes. I think it took much longer than I was expecting based on previous experience running the simpletest. But I'm not sure if this part is actually related to this issue since requests does pass nbytes and that is how this is intended to work as far as I understand.

[FoamyGuy]

I know it wasn't modified with this PR, but I'm also curious if you know anything about why this line is commented out?

Adafruit_CircuitPython_ESP32SPI/adafruit_esp32spi/adafruit_esp32spi_socket.py

Line 179 in 52208e4

# received.append(recv)

Could that have been commented out accidentally? or if it's not used is there any downside to removing it entirely?

[FoamyGuy]

I tried changing the while loop logic to >=:

while to_read >= limit:

When I ran it like that it seemed to get stuck in an infinite loop. I waited for several minutes, but the get request never completed (it didn't error out either just sat waiting).

[anecdata]

Instead of the limit, would something like this work?

if nbytes is a positive integer (or somesuch check):
    to_read = nbytes
else:
    to_read = len(buffer)

And then the rest could mostly mirror recv()

[anecdata]

Or... this may be a crazy suggestion (I'm not that expert at Python), but could it work to leverage recv inside recv_into, something like:

def recv_into(self, buffer, nbytes=None):
    self._buffer = buffer
    buffer = recv(nbytes)  # or 0 if nbytes is None
return len(some magic way to know how many bytes were actually put into the buffer; maybe a global)

if something like that could work, it would shorten and simplify the code, and reduce the surface area for bugs

...just found this example where it seems to be done like this:
zerotier/libzt#145

[tekktrik]

We could do that, but doesn't that defeat the purpose of using recv_into? I don't know how garbage collection works, and whether the concern for memory is more in this code or the user implementation. If that's the latter, than that's definitely the easiest way to do it, and if not at at the very least a good hotfix until it can be fleshed out.

[tekktrik]

Instead of the limit, would something like this work?

if nbytes is a positive integer (or somesuch check):
    to_read = nbytes
else:
    to_read = len(buffer)

And then the rest could mostly mirror recv()

This could work, I think we might need to change the return value to ensure it still returns the number bytes written

[anecdata]

My thought (perhaps erroneously) was to use the supplied buffer, but point to it in the global ._buffer so that the lower-level recv is putting bytes there and not re-allocating. Maybe it doesn't work that way.

Libraries and user code draw from the same memory, and you're right, recv_into is to allow the user to manage the allocation(s).

[jasonwashburn]

I think we still have some kind of issue related to this. When I run the requests simpletest on a PyPortal with 7.2.0-alpha-1 I am consistently getting this error raised:

code.py output:
Connecting to AP...
Connected to AloeVera   RSSI: -42
Fetching text from http://wifitest.adafruit.com/testwifi/index.html
Traceback (most recent call last):
  File "code.py", line 58, in <module>
  File "/lib/adafruit_requests.py", line 847, in get
  File "/lib/adafruit_requests.py", line 719, in request
  File "/lib/adafruit_requests.py", line 216, in __init__
RuntimeError: Unable to read HTTP response.

Code done running.

I tried several times and it always returns the same error, has never completed the request successfully for me.

I looked into this a bit during my stream today but was unable to determine the real root cause (the details of the buffer handling and internals of requests and esp32spi socket a bit over my head ATM).

I can confirm I'm also getting the same error using this PR and several versions of example code that tries to use requests.get on a Pyportal running 7.1.1

[anecdata]

probably want a check that if nbytes is supplied, it's positive (and non-zero??) and isn't greater than len(buffer), just to be safe

[tekktrik]

Deleted my last comment because oops, I'm wrong! - https://docs.python.org/3/library/socket.html#socket.socket.recv_into

I'll change the default input of recv_into() to be 0 and match this (and other libraries') implementations. I'll also add a check the number is within a valid range.

[tekktrik]

Also, good catch @FoamyGuy, that line should not have been commented out!

Simple tests still won't complete but it seems to be a different issue, I think with the adafruit_requests library.

[tekktrik]

I'm still working on why recv_into is causing issues for the adafruit_requests library but my current theory is that the issue is there. Doing some debugging to determine the exact issue still!

[jasonwashburn]

So, I was able to get requests_simpletest.py from the requests repository examples to run without producing errors using your latest commit. However, the first request consistently takes exactly 1min, 1sec to "complete", and only returns the first 23 chars. I added in some prints to show the time elapsed for each request.

Connecting to AP...
Connected to WBurn-2G   RSSI: -40
Fetching text from http://wifitest.adafruit.com/testwifi/index.html
----------------------------------------
Text Response:  This is a test of Adafr
----------------------------------------
Time elapsed: 0:01:01
Fetching JSON data from https://httpbin.org/get
----------------------------------------
JSON Response:  {'url': 'https://httpbin.org/get', 'headers': {'User-Agent': 'Adafruit CircuitPython', 'Host': 'httpbin.org', 'X-Amzn-Trace-Id': 'Root=1-61ecb914-1f47aa991e2377d45241cddf'}, 'args': {}, 'origin': '68.13.113.212'}
----------------------------------------
Time elapsed: 0:00:05
POSTing data to https://httpbin.org/post: 31F
----------------------------------------
Data received from server: 31F
----------------------------------------
Time elapsed: 0:00:01
POSTing data to https://httpbin.org/post: {'Date': 'July 25, 2019'}
----------------------------------------
JSON Data received from server: {'Date': 'July 25, 2019'}
----------------------------------------
Time elapsed: 0:00:01

Code done running.

Strangely though, if you change the plain text URL to http://www.google.com the results come back quickly, but the response is still cutoff, though after significantly more characters.

Fetching text from http://www.google.com
----------------------------------------
Text Response:  <!doctype html><html itemscope="" itemtype="http://schema.org/WebPage" lang="en"><head><meta content="Search the world's information, including webpages, images, videos and more. Google has many special features to help you find exactl
----------------------------------------
Time elapsed: 0:00:02

[tekktrik]

Thanks for the confirmation! I am also seeing a reduced payload "printout" response from the Adafruit-based request, but during my debugging, it does seem that it is actually getting that data but for some reason not returning it. I'm currently digging to see why that's the case.

[tekktrik]

Okay, this should be the fix! The issue was still with how the limit of data was calculated, causing the requests simpletest to fail. The fixes I pushed are what got me across the finish line and got it to work!

Fetching text from http://wifitest.adafruit.com/testwifi/index.html

This is a test of Adafruit WiFi!
If you can read this, its working :)

Fetching json from http://api.coindesk.com/v1/bpi/currentprice/USD.json

{'time': {'updated': 'Jan 24, 2022 03:04:00 UTC', 'updatedISO': '2022-01-24T03:04:00+00:00', 'updateduk': 'Jan 24, 2022 at 03:04
GMT'}, 'disclaimer': 'This data was produced from the CoinDesk Bitcoin Price Index (USD). Non-USD currency data converted
using hourly conversion rate from openexchangerates.org', 'bpi': {'USD': {'code': 'USD', 'description': 'United States Dollar',
'rate_float': 35262.0, 'rate': '35,262.0417'}}}

Done!

[anecdata]

I tested this out with some of my usual code and got confused because recv was still being used and hasattr(sock, "recv_into") was False. Turns out that when the Response is initialized, only some cases trigger the use of recv_into. For example, in the sample code in #154 only the first URL uses recv_into, the last three use recv.

In any case, the current PR code seems to work properly when recv_into is invoked.

FWIW, earlier I spent some time trying to get the concept of using recv within recv_into working, but got too deep in memoryview and type wrangling issues. But it later occurred to me that the Adafruit WIZnet5k library has a Python socket implementation and indeed it uses this approach. So I plugged that into ESP32SPI and it worked fine, though I believe it is making a local copy within the function. Maybe there's a way around this (probably needs coordinating changes in Requests). It may not be so bad as-it though since it's only 32 bytes at a time with two copies (in the Requests case).

    def recv_into(self, buf, nbytes=0, flags=0):
        """Reads some bytes from the connected remote address info the provided buffer.
        :param bytearray buf: Data buffer
        :param nbytes: Maximum number of bytes to receive
        :param int flags: ignored, present for compatibility.
        :returns: the number of bytes received
        """
        if nbytes == 0:
            nbytes = len(buf)
        ret = self.recv(nbytes)
        nbytes = len(ret)
        buf[:nbytes] = ret
        return nbytes

[FoamyGuy]

LGTM Thanks @tekktrik

I tested the latest version successfully on PyPortal 7.2.0-alpha.1

[tekktrik]

No problem! You bricks it, you fix it!