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

It's unfortunate that Response.close() tries to read all remaining information; buggy too #130

Closed
jepler opened this issue Mar 29, 2023 · 2 comments

Comments

@jepler
Copy link
Member

jepler commented Mar 29, 2023

Response.close() seems to try to read all remaining data out of the socket in order to work around a flaw or limitation in the ESP32SPI socket implementation.

There are two problems with this:

  • A client may want / need to close an ongoing request before reading all information in it
  • as implemented it's not necessarily exhausting all content, but at most one chunk in the chunked-encoding
    • this may not be possible, the stream could be infinite
  • handling of chunked mode is wrong and can lead to errors during close()

As far as the last point, I did experience it 'going wrong' with the chunked mode length being part of the http stream, but there's also the case where the exact content is carefully read (runs on desktop linux):

import socket
import ssl
import adafruit_requests

session = adafruit_requests.Session(socket, ssl.create_default_context())
with session.get('https://github.com') as response:
    assert response._chunked
    buf = bytearray(1)
    response._readinto(buf)
    print(buf)
    while response._remaining:
        print(response._remaining)
        response._readinto(buf)
        print(buf)
    assert response._cached is None
    assert response.socket

if there's a limitation of ESP32SPI that a socket has to be fully read before it's closed, can't we push that down into ESP32SPI (or better yet fix the limitation in nina-fw) and get rid of this trouble-causing block?

@justmobilize
Copy link
Collaborator

@jepler or @dhalbert can this be closed with my cleanup on response.close()

@dhalbert
Copy link
Contributor

Fixed by #159.

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

No branches or pull requests

3 participants