Skip to content

204 Fix #213

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 27, 2025
Merged

204 Fix #213

merged 1 commit into from
May 27, 2025

Conversation

justmobilize
Copy link
Collaborator

@justmobilize justmobilize commented May 24, 2025 •
edited
Loading

Resolves: #210

Looking at requests in Cpython: requests uses urllib3 which uses CPython http

in http.client during a HTTPConnection.getresponse it calls HTTPResponse.begin which does the following logic to determine to set the length to 0:

        # does the body have a fixed length? (of zero)
        if (status == NO_CONTENT or status == NOT_MODIFIED or
            100 <= status < 200 or      # 1xx codes
            self._method == "HEAD"):
            self.length = 0

This copies this logic and puts it into _parse_headers whcih is the closes thing we have.

@justmobilize justmobilize marked this pull request as ready for review May 24, 2025 23:18
justmobilize
justmobilize commented May 24, 2025
View reviewed changes
adafruit_requests.py
@@ -102,11 +102,12 @@ class Response:
It is still necessary to ``close`` the response object for correct management of
sockets, including doing so implicitly via ``with requests.get(...) as response``."""

def __init__(self, sock: SocketType, session: "Session") -> None:
def __init__(self, sock: SocketType, session: "Session", method: str) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FoamyGuy This is the only way we can also do the HEAD check (which would also have no content). Are we good with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

tests/real_call_test.py
import adafruit_requests


@pytest.mark.parametrize(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests, do actual calls to httpbin. We should make sure not to add too many, but this should help us make sure we don't break anything...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with limiting the use of calls out to 3rd party services in the tests. This one is okay for now, but perhaps it would be good to add a simple httpserver script in the tests dir that the tests can then interact with. That way it can get the same functionality but be more self contained, and not reliant on actual network conditions (I had httpbin returning 503 for a moment which I think would've caused the test to fail).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justmobilize This did end up causing the test and thus actions check to fail: https://github.com/adafruit/Adafruit_CircuitPython_Requests/actions/runs/15278252260/job/42970208909#step:2:1007

With that in mind I think rather just not have our tests rely on the 3rd party service. If you have a moment and can PR removing this test, or altering it to use a local server please do. I can circle back in a day or two and do it if you don't.

@justmobilize justmobilize requested a review from FoamyGuy May 24, 2025 23:19
FoamyGuy
FoamyGuy approved these changes May 27, 2025
View reviewed changes
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks good to me. I tested successfully on a Feather ESP32-S3 with slighly modified reproducer from the issue.

adafruit_requests.py
@@ -102,11 +102,12 @@ class Response:
It is still necessary to ``close`` the response object for correct management of
sockets, including doing so implicitly via ``with requests.get(...) as response``."""

def __init__(self, sock: SocketType, session: "Session") -> None:
def __init__(self, sock: SocketType, session: "Session", method: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

tests/real_call_test.py
import adafruit_requests


@pytest.mark.parametrize(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with limiting the use of calls out to 3rd party services in the tests. This one is okay for now, but perhaps it would be good to add a simple httpserver script in the tests dir that the tests can then interact with. That way it can get the same functionality but be more self contained, and not reliant on actual network conditions (I had httpbin returning 503 for a moment which I think would've caused the test to fail).

@FoamyGuy FoamyGuy merged commit d72cfb9 into adafruit:main May 27, 2025
1 check passed
@FoamyGuy FoamyGuy mentioned this pull request May 27, 2025
Closed
@justmobilize justmobilize deleted the 204-fix branch May 27, 2025 17:08
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request May 28, 2025
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@f23c9c4)
8e0b8d1 
Updating https://github.com/adafruit/Adafruit_CircuitPython_EPD to 2.13.4 from 2.13.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_EPD#88 from makermelissa/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_INA219 to 3.4.28 from 3.4.27:
  > Merge pull request adafruit/Adafruit_CircuitPython_INA219#39 from ilario/importerror_i2c

Updating https://github.com/adafruit/Adafruit_CircuitPython_INA3221 to 1.2.0 from 1.1.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_INA3221#8 from BZab/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_OPT4048 to 1.0.1 from 1.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_OPT4048#1 from FoamyGuy/use_properties

Updating https://github.com/adafruit/Adafruit_CircuitPython_OV5640 to 1.2.8 from 1.2.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_OV5640#40 from FoamyGuy/saturation_effect_fix

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1322 to 1.4.5 from 1.4.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1322#25 from FoamyGuy/displayio_api_update

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1327 to 1.4.5 from 1.4.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1327#27 from adafruit/update-display-modules

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1680 to 2.0.2 from 2.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1680#32 from makermelissa/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_ST7735 to 1.2.17 from 1.2.16:
  > Merge pull request adafruit/Adafruit_CircuitPython_ST7735#23 from FoamyGuy/displayio_api_update

Updating https://github.com/adafruit/Adafruit_CircuitPython_VL53L1X to 1.2.4 from 1.2.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_VL53L1X#25 from apzerafa/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_Dash_Dis to 3.0.2 from 3.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Dash_Dis#22 from adafruit/dhalbert-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 3.3.0 from 3.2.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#220 from cinderblockgames/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 4.1.11 from 4.1.10:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#213 from justmobilize/204-fix

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_OPT4048
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FoamyGuy FoamyGuy FoamyGuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Responses with no content cause timeout error and crash
2 participants
@justmobilize @FoamyGuy