Skip to content

Update HTTPClient.cpp to send big payload #7638

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

Closed

Conversation

ekapujiw2002
Copy link

Description of Change

This update will handle sending big payload packet correctly by sending it per determined chunk size, for example per 1000 bytes. Also fix content length header on subsequence redirection request.

Tests scenarios

Tested on ESP32-CAM AiThinker using Arduino-esp32 core v3.3.1 for sending photo as base64 payload to GDrive via Google App Script. If payload size is more than 16384 bytes then HTTPClient::sendRequest will fail.

Related links

None

Handle sending big payload data per 1000 bytes
@CLAassistant
Copy link

CLAassistant commented Dec 29, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

// }

// send the payload per 1000 bytes
log_d("Sending %u byte ...\n", size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like your PR is introducing a mix of TAB and SPACE characters resulting in a jumbled mess of formatting. Can you please reformat your additional code to match the existing formatting?

Copy link
Author

Choose a reason for hiding this comment

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

@atanisoft Fixed. Please kindly check it. Thank you.

if(_client->write(&payload[0], size) != size) {
return returnError(HTTPC_ERROR_SEND_PAYLOAD_FAILED);

// this will fail for big payload
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of leaving the old code in comments, it would be better to simply drop it.

Copy link
Author

Choose a reason for hiding this comment

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

@atanisoft Fixed. Please kindly check it. Thank you.

// send the payload per 1000 bytes
log_d("Sending %u byte ...\n", size);
size_t sent_bytes = 0;
for (size_t id_data = 0; id_data < size; id_data+=1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of a for loop using fixed sizes for chunks, please consider a loop similar to below (note this is not tested):

while(sent_bytes < size) {
  size_t sent = _client->write(&payload[sent_bytes], std::min(size - sent_bytes, _chunkSize));
  if (sent < 0) {
    log_e("Failed to send chunk!");
    break;
  }
  sent_bytes += sent;
}

Note that there is ZERO guarantee that the write call will send the full block size you request it to send thus you must check that it actually sent data and only then move the data starting point. The check for negative sent byte count is to check for sending errors.

Copy link
Author

Choose a reason for hiding this comment

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

@atanisoft Fixed. Please kindly check it. Thank you.

@VojtechBartoska VojtechBartoska added Status: Review needed Issue or PR is awaiting review Area: Libraries Issue is related to Library support. labels Jan 25, 2023
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.

@ekapujiw2002 Did you take a look on comments from @atanisoft? If not, please do the changes you were asked for.

@ekapujiw2002
Copy link
Author

@ekapujiw2002 Did you take a look on comments from @atanisoft? If not, please do the changes you were asked for.

@P-R-O-C-H-Y Will do sir ASAP. Thank you.

@me-no-dev
Copy link
Member

@ekapujiw2002 I invite you to see the changes that I have made on the topic of large uploads here. I have tested it and also added some more safeguards and error propagation.

@VojtechBartoska VojtechBartoska added this to the 3.0.0 milestone Nov 28, 2023
@me-no-dev
Copy link
Member

closing as already implemented

@me-no-dev me-no-dev closed this Dec 14, 2023
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
None yet
Development

Successfully merging this pull request may close these issues.

6 participants