Skip to content

Fix infinite loop when requesting too quickly #45

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

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Conversation

foxy82
Copy link
Contributor

@foxy82 foxy82 commented Apr 3, 2023

On the Pico W we can get in an infinite loop if we request pages too quickly. This is because we get an OSError 32 Broken Pipe back from which we can't recover. I've made it so any of the unexpected errors will be re-raised.

Fixes this issue:
#44

I also wonder if the same issue can happen in server._receive_header_bytes / server._receive_body_bytes which has very similar code that catches and swallows OSError unless it is ETIMEDOUT ?

foxy82 added 3 commits April 3, 2023 12:10
On the Pico W we can get in an infinite loop if we request pages too quickly. This is because we get an OSError 32 Broken Pipe back from which we can't recover.
I've made it so any of the unexpected errors will be re-raised.

Fixes this issue:
adafruit#44
Fix linting issue
Ensure unexpected OSError isn't swallowed.
@foxy82
Copy link
Contributor Author

foxy82 commented Apr 3, 2023

Whilst I can't re-create for the _receive_header_bytes / _receive_body_bytes it feels more correct to throw any unexpected errors but happy ot take advice if I'm misunderstanding something here.

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.

This seems okay to me. I tested on a a PicoW with 8.1 beta. Using the currently released version I did end up in the infinite loop / locked up state once by spam clicking the mute button on the index page.

Using the version from this PR I have not gotten back into that infinite loop state. All of my requests are succeeding, I spam clicked many times and even ran a python script with a tight loop making requests over and over the PicoW was able to handle all requests and return proper responses. I'm not certain if I just got "lucky" with the testing, but I never did have a raised error stop my server poll from continuing.

I don't necessarily understand the potential ramifications in the core, like is this really specific to picow, and if so is there a fix in the core that would be better, or should go alongisde this?

I'll wait for others to chime in on that, but if there is nothing further that needs done on that front then this seems good to me and I'll come back to approve if not.

@foxy82
Copy link
Contributor Author

foxy82 commented Apr 3, 2023

Thanks for testing.

I don't necessarily understand the potential ramifications in the core, like is this really specific to picow, and if so is there a fix in the core that would be better, or should go alongisde this?

Yeah it's worth understanding if the Broken Pipe is due to some other issue that needs to be fixed.

That being said catching OSError - and then checking for a couple of specific errors and swallowing any other error is at worst hiding issues but at best dangerous of getting in this infinite loop - so I think this change should still be done in case of any other OSErrors that may be thrown in the future.

@dhalbert
Copy link
Contributor

@FoamyGuy Shall we merge this too, including it in all the new changes?

@FoamyGuy
Copy link
Contributor

@dhalbert Yep sounds good, I'll merge this one as well. I made new release with the change from the other PR before I saw you're note here. But will make another new one after this merge.

@FoamyGuy FoamyGuy merged commit 0850aba into adafruit:main Apr 24, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 25, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_AGS02MA to 1.0.4 from 1.0.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_AGS02MA#3 from jposada202020/docs
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_CLUE to 3.0.14 from 3.0.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_CLUE#59 from julianaklulo/main
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_EPD to 2.11.0 from 2.10.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_EPD#61 from ladyada/master
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP2515 to 1.1.1 from 1.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_MCP2515#18 from adafruit/use-core-message-rtr-classes

Updating https://github.com/adafruit/Adafruit_CircuitPython_framebuf to 1.6.1 from 1.6.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_framebuf#52 from steka/improve_font_to_bin

Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 3.0.1 from 2.5.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#45 from foxy82/main
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#49 from michalpokusa/prevent-parent-directory-access

Updating https://github.com/adafruit/Adafruit_CircuitPython_Pixel_Framebuf to 1.1.10 from 1.1.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_Pixel_Framebuf#9 from RossK1/adding_type_hints
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_TinyLoRa to 2.2.13 from 2.2.12:
  > Merge pull request adafruit/Adafruit_CircuitPython_TinyLoRa#49 from awordforthat/issue47/fix-short-name-errors
  > Add upload url to release action
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
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.

3 participants