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

[HTTPD] support for multiple simultaneous requests (try 3) (IDFGH-9868) #11190

Conversation

chipweinberger
Copy link
Contributor

@chipweinberger chipweinberger commented Apr 13, 2023

Related Issue: #10594

Previous PR: #10669

Add basic support for supporting multiple simultaneous requests in HTTPD server.

This is the 3rd try of this PR:

  • 1st: worked well
  • 2nd: tried to simplified the user api, but made things too complicated for the server code
  • 3rd: went back to the first approach, with slightly cleaner api

Note: This PR does not add automatic support for simultaneous requests, but instead makes it more ergonomic for users to move a request to a thread that they must create themselves.

Advantages over raw sockets

  • easier reply: Users can use all the existing httpd functions (i.e. httpd_resp_sendstr) to send their response instead of raw socket send()
  • LRU safety: A long running async socket will not get closed from underneath us if we run out of sockets

Implementation:

  • an request handler will first call httpd_req_async_handler_begin()
    • this gives the user exclusive use of a new request object (to not interfere with the http server's request)
    • this will mark the socket as "in use" to guarentee the socket to not be LRU purged
  • async handler must call httpd_req_async_handler_complete() when done
    • this frees our request object.
    • this will mark the socket as "no longer in use" to allow it to be closed (LRU purged)

Example

  • add an example to ESP-IDF, showing usage.

@chipweinberger chipweinberger force-pushed the user/chip/http-multiple-requests-try-3 branch 8 times, most recently from 7c3c379 to 16ea023 Compare April 13, 2023 21:59
@espressif-bot espressif-bot added the Status: Opened Issue is new label Apr 13, 2023
@github-actions github-actions bot changed the title [HTTPD] support for multiple simultaneous requests (try 3) [HTTPD] support for multiple simultaneous requests (try 3) (IDFGH-9868) Apr 13, 2023
@chipweinberger chipweinberger force-pushed the user/chip/http-multiple-requests-try-3 branch from 16ea023 to ead5f42 Compare April 14, 2023 00:05
@chipweinberger
Copy link
Contributor Author

@hmalpani

Copy link
Contributor

@hmalpani hmalpani left a comment

Choose a reason for hiding this comment

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

Hello @chipweinberger
This approach looks simpler to me.
Although when I tried these changes, I got into some errors. The program crashed when calling long handler. Can you please check and update the PR accordingly?
Thanks!

examples/protocols/http_server/async_handlers/main/main.c Outdated Show resolved Hide resolved
@chipweinberger chipweinberger force-pushed the user/chip/http-multiple-requests-try-3 branch from ead5f42 to d9bf0ac Compare April 17, 2023 22:33
@chipweinberger
Copy link
Contributor Author

chipweinberger commented Apr 17, 2023

Pushed! It works now.

If youre curious the fix, I had introduced a small bug right before pushing. 🤦

in httpd_req_async_handler_begin I was calling a memcpy out of order, overwriting a previous assignment.

@chipweinberger chipweinberger force-pushed the user/chip/http-multiple-requests-try-3 branch 6 times, most recently from 123a7d1 to 97c3883 Compare April 17, 2023 22:54
@chipweinberger chipweinberger force-pushed the user/chip/http-multiple-requests-try-3 branch from 97c3883 to 1289d69 Compare April 17, 2023 22:59
@hmalpani
Copy link
Contributor

sha=1289d69870dc5faee84756e168dbacdb59f62a4a

@hmalpani hmalpani added the PR-Sync-Merge Pull request sync as merge commit label Apr 19, 2023
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2023

CLA assistant check
All committers have signed the CLA.

@hmalpani
Copy link
Contributor

Hello @chipweinberger
Can you please sign the CLA agreement. Thanks!

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Apr 24, 2023
@chipweinberger
Copy link
Contributor Author

signed

@chipweinberger
Copy link
Contributor Author

any updates? thanks

@hmalpani
Copy link
Contributor

Hello @chipweinberger
I have created an internal MR which is under review. It will get reflected on GitHub once it gets merged.
Thanks!

@chipweinberger
Copy link
Contributor Author

chipweinberger commented May 9, 2023

perhaps I can split the PR up into 2 to make it reviewed faster? Let me know if that would be better.

  1. core changes (which is only a small diff in 4 files)
  2. new example

@hmalpani
Copy link
Contributor

perhaps I can split the PR up into 2 to make it reviewed faster? Let me know if that would be better.

1. core changes (which is only a small diff in 4 files)

2. new example

Hello @chipweinberger
The PR is already in review and should not take long before getting merged. So no need to update the PR.
Thanks!

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: In Progress Work is in progress labels May 21, 2023
@hmalpani
Copy link
Contributor

PR has been merged into master via commit: 3824eba

Closing this PR.

@hmalpani hmalpani closed this May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants