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

Fix hangup on reading of closed socket #151

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Conversation

pinkavaj
Copy link
Contributor

@pinkavaj pinkavaj commented Mar 3, 2024

We cannot get more date on closed socket, return what we have, if anythink.

Closes #135

We cannot get more date on closed socket, return what we have, if anythink
@FoamyGuy
Copy link
Contributor

This makes sense as far as I can tell but I lack the specific knowledge about the lower level details so I may not be the best to review.

I did test this branch successfully with the simpletest script in this repo on a Feather RP2040 + Ethernet Featherwing.

I attempted to test the specific path of code that contains the change but couldn't figure out how. I tried adding some sleep inside of recv_into (to give myself more time to react) and then unplugging the ethernet cable while a request was being received. I'm not really sure if that is a correct way to test this, but in my case it was raising this exception from inside the if block that this elif was added to rather than making it to the elif and using the new code.

Traceback (most recent call last):
  File "code.py", line 23, in <module>
  File "/lib/adafruit_wiznet5k/adafruit_wiznet5k.py", line 262, in __init__
  File "/lib/adafruit_wiznet5k/adafruit_wiznet5k.py", line 276, in set_dhcp
  File "/lib/adafruit_wiznet5k/adafruit_wiznet5k_dhcp.py", line 183, in request_dhcp_lease
  File "/lib/adafruit_wiznet5k/adafruit_wiznet5k_dhcp.py", line 426, in _dhcp_state_machine
  File "/lib/adafruit_wiznet5k/adafruit_wiznet5k_dhcp.py", line 343, in _handle_dhcp_message
  File "/lib/adafruit_wiznet5k/adafruit_wiznet5k.py", line 562, in socket_connect
  File "/lib/adafruit_wiznet5k/adafruit_wiznet5k.py", line 1060, in _check_link_status
ConnectionError: The Ethernet connection is down.

@justmobilize
Copy link
Collaborator

I think to test, we would need a way to kill the socket from the other side. I wonder if you setup one device as a webserver, and then during that wait, power that device off, if you would get the right thing (because then the socket would be closed). You could also spin up something local if that's easier.

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 @justmobilize! That did the trick.

I tested this successfully with this flask handler:

def hello():
    print("inc request to hello")
    time.sleep(10)
    return "Hot Peppers"

During the sleep() I stopped the server with keyboard interrupt.

Using this branch the microcontroller notices the severed connection immediately raising RuntimeError: Error connecting socket: Failed to establish connection.

Using current main there is a very long delay and then eventually the request times out.

The version from this branch responds much faster, pretty much instantly when the flask server is stopped.

@FoamyGuy FoamyGuy merged commit f78907e into adafruit:main Apr 22, 2024
1 check passed
@pinkavaj pinkavaj deleted the pi-fix-recv branch April 22, 2024 22:46
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 23, 2024
Updating https://github.com/adafruit/Adafruit_CircuitPython_floppy to 2.0.0 from 1.0.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_floppy#11 from adafruit/updates-incompatible-floppyio-floppsy

Updating https://github.com/adafruit/Adafruit_CircuitPython_SharpMemoryDisplay to 1.4.12 from 1.4.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_SharpMemoryDisplay#24 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_Thermistor to 3.4.11 from 3.4.10:
  > Merge pull request adafruit/Adafruit_CircuitPython_Thermistor#22 from adafruit/dhalbert-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 5.2.1 from 5.0.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#151 from pinkavaj/pi-fix-recv
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#157 from adafruit/core-compatible-socket-type-numbers
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#153 from us3r64/fix/socket-httpserver-poll
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#149 from pinkavaj/pi-fix-close-wait
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#156 from kevin-tritz/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE to 10.0.7 from 10.0.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#195 from adafruit/dhalbert-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_BusDevice to 5.2.9 from 5.2.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_BusDevice#99 from RetiredWizard/spicsinit

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
aggieNick02 added a commit to PCPartPicker/Adafruit_CircuitPython_Wiznet5k that referenced this pull request Oct 10, 2024
failures.

The issue is caused by the code checking socket status *after* checking
if bytes are available. Bytes may have become available between the
last byte available check and the socket status check. This causes us to
incorrectly ignore the newly available bytes and return 0 to the caller,
which in any blocking/timeout scenario tells the caller no more bytes
will ever be available.

This was introduced in commit 89b9f10
See also
adafruit#151
as a fix for
adafruit#135
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

Successfully merging this pull request may close these issues.

recv hangs if there’s no timeout and connection closes
3 participants