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

Update to ESP8266HTTPClient.cpp for no Content-Length #7691

Merged
merged 9 commits into from Nov 20, 2020

Conversation

drderiv
Copy link
Contributor

@drderiv drderiv commented Nov 4, 2020

Response bodies are ignored when _transferEncoding == HTTPC_TE_IDENTITY and there is no Content-Length header. The added code here attempts to fix that issue by writing the response body based upon how much content is available to be read.

edit:
Fixes #7692

Response bodies are ignored when _transferEncoding == HTTPC_TE_IDENTITY and there is no Content-Length header.  The added code here fixes that issue.
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Little minor refactor, please.

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup.

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 15, 2020

Now that it's working but with an unwilling timeout, you can update writetostreamdatablock() for it to stop when it can no longer read from the _client.
This is an old fashion function. A that time there were usually several attempts to read or write before giving up. Today one attempt and timeout checking is enough. This is addressed by #6979.
In the meantime, what is missing there is checking peer connectivity. That is answering to that question:
Is it possible to get more incoming data ? One just need to check _client's ::available()==0 and (::connected()==false or ::status() != WL_CONNECTED). When this is true, it is no longer useful to wait for the timeout, peer has closed the connection and everything that is to be read is already swallowed.

@d-a-v d-a-v added this to the 3.0.0 milestone Nov 16, 2020
@d-a-v d-a-v added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Nov 16, 2020
@drderiv
Copy link
Contributor Author

drderiv commented Nov 16, 2020

With apologies, I'm not seeing how this would help, at least in the current implementations of HTTPClient and Stream. My reading is that, in a no Content-Length scenario (so len = -1), once readBytes() is called, the timeout controls, regardless of anything else. If that's correct, checking peer or wifi connectivity within writeToStreamDataBlock would not be useful. Changes to address this would need to occur within Stream, and then writeToStreamDataBlock would need to be modified to check the parameters you mention. In addition, I would still think a timeout would be needed (and it would have to be added to writeToStreamDataBlock as well) in the case where you remain connected but the server just decides to take a nap.

Perhaps the word 'meantime' threw me and you are asking for the HTTPClient mods prospectively to work with the changes to Stream implemented in #6979?

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 16, 2020

In #6979, writetostreamdatablock is removed and replaced by Stream::to() - having both timeout and end of stream detection - but still lacks from maintainers's review because it was introducing Stream:: inheritance to String::, now removed, that was considered by some as a blaspheme, which I still don't understand why ;) Maybe I should repost it with another name and its history expunged of the heresy.

But before #6979 is considered, a fix is needed for your issue, and #7692 too which is the same issue.

End of TCP connection cannot currently be handled by Stream:: so it has to be writetostreamdatablock()'s job. While examining #7692 I ended up with the following and stopped there because I was too optimistic regarding #6979 consideration. You may try it. The logic is the following: do not ask too much to Stream::readBytes() because that one will wait until timeout that all requested data are received. Instead ask for what's already there and keep retrying until all data are received.

+++ b/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp
@@ -725,7 +725,7 @@ int HTTPClient::writeToStream(Stream * stream)
     int ret = 0;
 
     if(_transferEncoding == HTTPC_TE_IDENTITY) {
-        if(len > 0) {
+        if(len > 0 || len == -1) {
             ret = writeToStreamDataBlock(stream, len);
 
             // have we an error?
@@ -1184,6 +1184,9 @@ int HTTPClient::writeToStreamDataBlock(Stream * stream, int size)
         if(readBytes > buff_size) {
             readBytes = buff_size;
         }
+        int av = _client->available();
+        if (readBytes < 0 || readBytes > av)
+            readBytes = av;
 
         // read data

If this works for you, please update your PR.

Add logic to writeToStreamDataBlock to only read what's available so as to avoid timeout, and adjust formatting.
@drderiv
Copy link
Contributor Author

drderiv commented Nov 16, 2020

That worked nicely. And now I understand what you meant about effectively checking _client's ::available()==0 and ::connected()==false. The idea about reading what's already there was what I was trying to do -- poorly, as I didn't include the 'until all data are received' part -- in my initial PR. With your help, I think we've now got it right. Thanks again.

@earlephilhower earlephilhower merged commit 47a57e1 into esp8266:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http.getString() return blank
3 participants