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

Handle large octet-stream #9430

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Conversation

nathannau
Copy link
Contributor

@nathannau nathannau commented Mar 28, 2024

Issue

In scenarios where the request body size exceeds the available RAM, the ESP fails to allocate sufficient memory to process the request in one go.

Change

Now, when the "Content-Type" of the request is "application/octet-stream", it will attempt to call the "ufn" function of the handler, similar to how it handles multipart requests.

Code Example:

server.on(uri, HTTP_PUT, finalCallback, [](){
  HTTPRaw raw = server.raw();
  if (raw.status == RAW_START) {
    ...
  } else if (raw.status == RAW_WRITE) {
    ...
  } else if (raw.status == RAW_END) {
    ...
  }
});

@me-no-dev
Copy link
Member

This is much better now :)

@me-no-dev
Copy link
Member

BTW I see that you target the 2.x branch. Do you plan to make PR to master also? Else this feature would exist only on 2.0.x versions

@nathannau
Copy link
Contributor Author

It would be nice to do the same PR for the master, but I'm using PlatformIO and I can't target the master. And I didn't want to work on the master if this PR wasn't validated. 😄

@me-no-dev
Copy link
Member

Code looks good. Can you please add slightly better description and also a small example to show how this feature can be used.

@nathannau
Copy link
Contributor Author

I added an exemple and improved the description.

@me-no-dev
Copy link
Member

@P-R-O-C-H-Y could you give this a shot and also evaluate the port to 3.0.0?

@me-no-dev
Copy link
Member

because it's changing the current behavior, I lean towards having this for 3.0.0 and not for 2.0.15. We do not want to break people's code in a bugfix release. Major v3 is a much better fit

@me-no-dev
Copy link
Member

@Jason2866 could you help @nathannau try this on 3.0.0 and PIO?

@Jason2866
Copy link
Collaborator

@nathannau To test a PR with Platformio against latest master. -> Make a branch with your changes from branch master and do the following setup in Platformio

[env:esp32]
board = esp-wrover-kit
platform = https://github.com/platformio/platform-espressif32.git
platform_packages =
	platformio/framework-arduinoespressif32 @ https://github.com/nathannau/arduino-esp32.git#your_branch_name
	platformio/framework-arduinoespressif32-libs @ https://github.com/espressif/esp32-arduino-libs.git#idf-release/v5.1

@nathannau
Copy link
Contributor Author

I'm currently testing whether the "Content-Type" is "Application/octet-stream".
It might be better to simply test that it's not a form. This would make it possible to generalise the processing of request bodies as you go along, when you don't want to load everything into memory.

@me-no-dev
Copy link
Member

@nathannau it's important that current behavior is not changed much for the average folk. Simple request should go fine as they do right now and use raw only when wanted to.

@nathannau
Copy link
Contributor Author

@me-no-dev Exactly. This patch would retain backwards compatibility while adding the ability to do more things if needed.

Copy link
Collaborator

@lucasssvaz lucasssvaz left a comment

Choose a reason for hiding this comment

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

Just some fixes

Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

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

Tested with @lucasssvaz fix which is needed. After the fix we can merge.

@P-R-O-C-H-Y P-R-O-C-H-Y added the Area: Libraries Issue is related to Library support. label Apr 3, 2024
@VojtechBartoska VojtechBartoska added this to the 2.0.15 milestone Apr 3, 2024
@VojtechBartoska VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Apr 3, 2024
@lucasssvaz lucasssvaz added Resolution: Awaiting response Waiting for response of author and removed Status: Review needed Issue or PR is awaiting review labels Apr 3, 2024
@me-no-dev
Copy link
Member

@nathannau this is currently blocking 2.0.15 release. Is there any chance that you take care of the notes from @lucasssvaz ? If you do not have the time, we could do that for you as well and merge the PR

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: Review needed Issue or PR is awaiting review and removed Resolution: Awaiting response Waiting for response of author labels Apr 7, 2024
@me-no-dev me-no-dev merged commit 7d911b9 into espressif:release/v2.x Apr 8, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Libraries Issue is related to Library support. Status: Review needed Issue or PR is awaiting review
Projects
Development

Successfully merging this pull request may close these issues.

6 participants