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

RFC: Web Workflow reliability and performance improvements #7836

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

thess
Copy link

@thess thess commented Apr 5, 2023

What started as an investigation of un-explained 403 errors from what I think
is a questionable CORS implementation along with interface hangs and connection
timeouts, I would like to propose the attached changes to the web_workflow
service. [Note: The CORS implementation is another topic not covered here.]

Observed problems (ESP32 platforms):

  1. Listening socket closed at lower level lwip resets not re-opened.
  2. User sockets being added to socket select fd lists causing errors.
  3. REST HTTP recv/parsing character-at-a-time causing select thread to continuously
    run and attempt to post background task requests.
  4. Active HTTP request socket not being closed or believed to be closed when it
    is still in use by select polling thread.
  5. HTTP request response timeouts due to looping background tasks.

These changes address, in no particular order:

  • Fixes polling thread looping forever hangs preventing new connections.
  • Don't lose listening sockets on mp resets and re-init.
  • Keep better separation of "system" and "user" sockets.
  • Track socket states to prevent re-use of sockets before closed.
  • Close REST socket when transaction completes. No post-init.
  • Remove unnecessary/redundant state flags.

There may be other issues here and I am hoping for some more eyes/testing on these proposed
changes. I have been running these changes for over 2-weeks on an UMFeatherS2 now with
no loss of service. The device sleeps for 20min, then wakes and takes a number of
sensor readings and posts them to a MQTT server and adafruit.io and goes back to sleep.
The device also polls the MQTT server for notice to not sleep in order to allow connection
via Web Workflow for maintenance.

After stabilizing the ESP32 environment, I have testing these changes on the RPi
implementation. The Web Workflow processing model works fine now with both the ESP32
and RPi given the significant differences in socket handling (polling vs. callbacks).
No further work was required to the RPi socket layer as was necessary with the ESP32
sockets/lwip layer. In fact, the ESP32 implementation seems to be a lot more responsive
and on-par with the RPi now.

Copy link
Member

@tannewt tannewt 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 so much for these improvements. I just have a couple of questions before merging.

ports/espressif/common-hal/socketpool/Socket.c Outdated Show resolved Hide resolved
supervisor/workflow.h Outdated Show resolved Hide resolved
supervisor/shared/workflow.c Outdated Show resolved Hide resolved
@tannewt tannewt added enhancement espressif applies to multiple Espressif chips web workflow labels Apr 5, 2023
@tannewt tannewt added this to the 8.1.0 milestone Apr 5, 2023
Fixes polling thread looping forever hangs preventing new connections.
Don't lose listening sockets on mp resets and re-init.
Keep better separation of "system" and "user" sockets.
Track socket states to prevent re-use of sockets before closed.
Close REST socket when transaction completes. No post-init.
Remove unnecessary state flags.
@thess thess requested a review from tannewt April 5, 2023 17:58
Copy link
Member

@tannewt tannewt 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!

@microdev1 microdev1 merged commit 50e259f into adafruit:main Apr 7, 2023
@thess thess deleted the rfc-web-server branch April 22, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement espressif applies to multiple Espressif chips web workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants