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

Fixes large payload issues for sendRequest #7051

Merged
merged 1 commit into from
Feb 1, 2020

Conversation

ramirocarra
Copy link
Contributor

@ramirocarra ramirocarra commented Jan 31, 2020

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

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
Copy link
Collaborator

@d-a-v d-a-v left a comment

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)

@d-a-v d-a-v added this to the 2.7.0 milestone Feb 1, 2020
@ramirocarra
Copy link
Contributor Author

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
Copy link
Collaborator

d-a-v commented Feb 1, 2020

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
Copy link
Contributor Author

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
Copy link
Collaborator

d-a-v commented Feb 1, 2020

keeping HTTP_TCP_BUFFER_SIZE is an unnecessary double verification?

It's more of a History/Legacy leftover.

@devyte
Copy link
Collaborator

devyte commented Feb 1, 2020

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

@devyte devyte merged commit 378c1f1 into esp8266:master Feb 1, 2020
@TD-er
Copy link
Contributor

TD-er commented Feb 4, 2020

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
Copy link
Collaborator

d-a-v commented Feb 4, 2020

This is unrelated because pubsubclient does not use HTTPClient.

@TD-er
Copy link
Contributor

TD-er commented Feb 4, 2020

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESP8266HTTPClient issue in library
4 participants