-
Notifications
You must be signed in to change notification settings - Fork 74
Allow optional nbytes parameter for recv_into #155
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
received = [] | ||
while to_read > 0: | ||
while to_read > limit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
Could that have been commented out accidentally? or if it's not used is there any downside to removing it entirely? |
I tried changing the while loop logic to >=:
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). |
Instead of the if nbytes is a positive integer (or somesuch check):
to_read = nbytes
else:
to_read = len(buffer) And then the rest could mostly mirror |
Or... this may be a crazy suggestion (I'm not that expert at Python), but could it work to leverage 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: |
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. |
This could work, I think we might need to change the return value to ensure it still returns the number bytes written |
My thought (perhaps erroneously) was to use the supplied buffer, but point to it in the global Libraries and user code draw from the same memory, and you're right, |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 |
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 |
I'm still working on why |
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.
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.
|
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. |
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!
|
I tested this out with some of my usual code and got confused because In any case, the current PR code seems to work properly when FWIW, earlier I spent some time trying to get the concept of using 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks @tekktrik
I tested the latest version successfully on PyPortal 7.2.0-alpha.1
No problem! You bricks it, you fix it! |
Updating https://github.com/adafruit/Adafruit_CircuitPython_24LC32 to 1.0.1 from 1.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_24LC32#11 from tekktrik/doc/add-typing > Merge pull request adafruit/Adafruit_CircuitPython_24LC32#12 from tekktrik/hotfix/patch-cleanup-fix Updating https://github.com/adafruit/Adafruit_CircuitPython_ADS1x15 to 2.2.11 from 2.2.10: > First part of patch > Merge pull request adafruit/Adafruit_CircuitPython_ADS1x15#77 from nlantau/patch-1 > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_BNO055 to 5.3.3 from 5.3.2: > Merge pull request adafruit/Adafruit_CircuitPython_BNO055#90 from adafruit/patch-fix > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_CCS811 to 1.3.6 from 1.3.5: > Merge pull request adafruit/Adafruit_CircuitPython_CCS811#47 from sti320a/patch-1 > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_CLUE to 3.0.2 from 3.0.1: > Merge pull request adafruit/Adafruit_CircuitPython_CLUE#54 from kevinjwalters/sample-fix > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 4.0.0 from 3.6.0: > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#146 from tekktrik/fix/rename-pin-args > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#155 from tekktrik/hotfix/fix-recv-into > First part of patch Updating https://github.com/adafruit/Adafruit_CircuitPython_SCD4X to 1.2.2 from 1.2.1: > Merge pull request adafruit/Adafruit_CircuitPython_SCD4X#12 from KeithTheEE/get-data-ready-status-fix > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_STMPE610 to 1.3.0 from 1.2.7: > Merge pull request adafruit/Adafruit_CircuitPython_STMPE610#22 from CedarGroveStudios/main > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_Thermistor to 3.4.0 from 3.3.8: > Merge pull request adafruit/Adafruit_CircuitPython_Thermistor#19 from raquo/main > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_Bitmap_Font to 1.5.5 from 1.5.4: > Merge pull request adafruit/Adafruit_CircuitPython_Bitmap_Font#56 from tekktrik/feature/ignore-comments-parsing > First part of patch Updating https://github.com/adafruit/Adafruit_CircuitPython_Debouncer to 1.4.0 from 1.3.13: > Merge pull request adafruit/Adafruit_CircuitPython_Debouncer#34 from prplz/main > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Button to 1.6.2 from 1.6.1: > Merge pull request adafruit/Adafruit_CircuitPython_Display_Button#34 from adafruit/patch-fix > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_Logging to 3.7.4 from 3.7.3: > Merge pull request adafruit/Adafruit_CircuitPython_Logging#25 from JingleheimerSE/add-time-format-specifier > First part of patch Updating https://github.com/adafruit/Adafruit_CircuitPython_MacroPad to 2.0.4 from 2.0.3: > Merge pull request adafruit/Adafruit_CircuitPython_MacroPad#34 from adafruit/patch-fix > First part of patch > update rtd py version Updating https://github.com/adafruit/Adafruit_CircuitPython_Simple_Text_Display to 1.2.2 from 1.2.1: > Merge pull request adafruit/Adafruit_CircuitPython_Simple_Text_Display#10 from adafruit/patch-fix > First part of patch > update rtd py version
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.