Fixes large payload issues for sendRequest by ramirocarra · Pull Request #7051 · esp8266/Arduino (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

ramirocarra

sendRequest has a major problem when sending a big payload, the comparator in the IF loop has its two operators changed at the same time, so the last part of payload is never sent. This is seen when you try to send a payload that needs more than one operation.

The modification has been tested for a 1500 bytes payload.

edit from maintainer: fixes #7049

@ramirocarra

sendRequest has a major problem when sending a big payload, the comparator in the IF loop has its two operators changed, so the last part of payload is never sent

d-a-v

Choose a reason for hiding this comment

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

Approving (even if it could be more simple by just commenting size -= written like suggested in #7049)

@ramirocarra

commenting size -= written may work, but that will make the "amount to write" wrong for the last chunk, in line:

int towrite = std::min((int)size, (int)HTTP_TCP_BUFFER_SIZE);

It may take some random info from payload pointer

@d-a-v

That is very true. But HTTP_TCP_BUFFER_SIZE is totally useless and we should have
towrite = size - byteswritten there. This is the kind of code that is coded and recoded many times through the core than can be simplified by one transfer function for all.

(I'm still approving your PR, other maintainers will comment)

@ramirocarra

That´s right too. I haven´t ckeck the code in function _client->write() but i guess it will take only the amount of info the buffer size allows (keeping HTTP_TCP_BUFFER_SIZE is an unnecessary double verification?)

@d-a-v

keeping HTTP_TCP_BUFFER_SIZE is an unnecessary double verification?

It's more of a History/Legacy leftover.

@devyte

This will likely get simplified with #6979 , but merging for the immediate need.

@TD-er

Is it possible this also may fix an issue I was having with running out of memory after a number of failed connection attempts to an MQTT broker?

@d-a-v

This is unrelated because pubsubclient does not use HTTPClient.

@TD-er

OK, thanks, that does limit my search to the real problem that may have been fixed now :)

timex-cme referenced this pull request

Feb 12, 2020

@earlephilhower @devyte

This is all @dirkx , whose PR unfortunately got borked when we were trying to update it to the new format. As @dirkx said:

When sending POST responses of well over a K - _write() may not sent it all. Make sure we do -- but cap the individual writes - as somehow large 2-3k blobs seem to cause instability (on my 12F units).

Supercedes #2528