Skip to content

HTTPClient writeToStream times out with 3.0.0 #8055

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
TheNitek opened this issue May 19, 2021 · 11 comments · Fixed by #8167
Closed

HTTPClient writeToStream times out with 3.0.0 #8055

TheNitek opened this issue May 19, 2021 · 11 comments · Fixed by #8167
Assignees
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Milestone

Comments

@TheNitek
Copy link
Contributor

TheNitek commented May 19, 2021

Platform

  • Hardware: ESP-12
  • Core Version: v3.0.0
  • Development Env: Platformio
  • Operating System: Windows

Settings in IDE

  • Module: Generic ESP8266 Module

Problem Description

Using HTTPClients writeToStream with 3.0.0 fails with a timeout, while the same code worked fine with 2.6.3
Increasing the timeout (e.g. setTimeout(10000)) does not help, but increase the time it takes until the timeout happens.

MCVE Sketch

#include <Arduino.h>

void setup() {
  [connect to WiFi]
  HTTPClient httpClient;
  std::unique_ptr<BearSSL::WiFiClientSecure>client(new BearSSL::WiFiClientSecure);
  client->setInsecure();

  httpClient.begin(*client, "https://mediandr-a.akamaihd.net/download/podcasts/podcast4223/AU-20210510-1944-4900.mp3");
  int httpCode = httpClient.GET();
  if (httpCode != HTTP_CODE_OK) {
    Serial.printf("invalid response code: %d\n", httpCode);
    httpClient.end();
    return;
  }

  sdfat::File32 episodeFile;
  if(!episodeFile.open("/download.tmp", sdfat::O_WRITE | sdfat::O_CREAT | sdfat::O_TRUNC)) {
    Serial.println(F("Failed to open target file"));
    httpClient.end();
    return;
  }

  int byteCount = httpClient.writeToStream(&episodeFile);
  Serial.printf("Downloaded %d bytes\n", byteCount);
  httpClient.end();

  episodeFile.close();
}

void loop() {

}

Debug Messages

[HTTP-Client][begin] url: https://mediandr-a.akamaihd.net/download/podcasts/podcast4223/AU-20210510-1944-4900.mp3
[HTTP-Client][begin] host: mediandr-a.akamaihd.net port: 443 url: /download/podcasts/podcast4223/AU-20210510-1944-4900.mp3
[HTTP-Client][sendRequest] type: 'GET' redirCount: 0
[HTTP-Client] connected to mediandr-a.akamaihd.net:443
[HTTP-Client] sending request header
-----
GET /download/podcasts/podcast4223/AU-20210510-1944-4900.mp3 HTTP/1.1
Host: mediandr-a.akamaihd.net
User-Agent: ESP8266HTTPClient
Accept-Encoding: identity;q=1,chunked;q=0.1,*;q=0
Connection: close
Content-Length: 0

-----
'HTTP-Client][handleHeaderResponse] RX: 'HTTP/1.1 200 OK
'HTTP-Client][handleHeaderResponse] RX: 'Accept-Ranges: bytes
'HTTP-Client][handleHeaderResponse] RX: 'Content-Type: audio/mpeg
'HTTP-Client][handleHeaderResponse] RX: 'ETag: "7e7cac4951e792fad423f87835737979:1620903727.463782"
'HTTP-Client][handleHeaderResponse] RX: 'Last-Modified: Thu, 13 May 2021 11:02:07 GMT
'HTTP-Client][handleHeaderResponse] RX: 'Server: AkamaiNetStorage
'HTTP-Client][handleHeaderResponse] RX: 'Content-Length: 20304601
'HTTP-Client][handleHeaderResponse] RX: 'Date: Wed, 19 May 2021 17:58:13 GMT
'HTTP-Client][handleHeaderResponse] RX: 'Connection: close
'HTTP-Client][handleHeaderResponse] RX: 'Access-Control-Max-Age: 86400
'HTTP-Client][handleHeaderResponse] RX: 'Access-Control-Allow-Credentials: false
'HTTP-Client][handleHeaderResponse] RX: 'Access-Control-Allow-Headers: *
'HTTP-Client][handleHeaderResponse] RX: 'Access-Control-Allow-Methods: GET,POST
'HTTP-Client][handleHeaderResponse] RX: 'Access-Control-Allow-Origin: *
'HTTP-Client][handleHeaderResponse] RX: '
[HTTP-Client][handleHeaderResponse] code: 200
[HTTP-Client][handleHeaderResponse] size: 20304601
pm open,type:2 0
[HTTP-Client][returnError] error(-11): read Timeout
[HTTP-Client][returnError] tcp stop
Downloaded -11 bytes
[HTTP-Client][end] tcp is closed
@d-a-v d-a-v self-assigned this May 19, 2021
@TheNitek
Copy link
Contributor Author

After further debugging:
StreamSend.SendGenericPeekBuffer times out because SDFat doesn't seem to override availableForWrite(), so it will return 0 everytime and no write will ever be attempted.

@d-a-v
Copy link
Collaborator

d-a-v commented May 22, 2021

I think @earlephilhower 's fork made for this core already helps.
It seems little is needed for ::availableForWrite() to be implemented

@TheNitek
Copy link
Contributor Author

TheNitek commented May 22, 2021

I guess this would degrade performance by quite a lot

Wouldn't it be an option to replace

w = std::min(w, avpk);

with
w = w ? std::min(w, avpk) : avpk;

Comment for ::availableForWrite() is
// default to zero, meaning "a single write may block"
// should be overriden by subclasses with buffering
so always returning 0 seems to be valid case which is not handled correctly by ::SendGenericPeekBuffer

@d-a-v
Copy link
Collaborator

d-a-v commented May 22, 2021

0 is a valid and possible value.
For example network can be blocking for immediate send, and FS can tell this way that SDcard is full.
Maybe @earlephilhower has some ideas about SdFat and availableforWrite() performance ?

@TheNitek
Copy link
Contributor Author

But that means that if Print* is like "I can never guarantee a non-blocking write" (i.e. always return 0) it can never be used as the target of a stream. That does not sound correct to me.

@d-a-v
Copy link
Collaborator

d-a-v commented May 22, 2021

That's true. So availableForWrite() has to be always implemented. It should be pure virtual.
Arduino API is a very sensible beast to touch though.

The stream::send API is currently unusable when ::availableForWrite() is not implemented.
It could be updated for such cases. Let's first try to solve the SdFat issue.

@TheNitek
Copy link
Contributor Author

I think @earlephilhower 's fork made for this core already helps.
It seems little is needed for ::availableForWrite() to be implemented

I just took a closer look at this and it looks like performance impact might be nearly not existing. So I'll give this a try.

@TheNitek
Copy link
Contributor Author

::availableSpaceForWrite() also returns 0 all the time, returning at this line:
https://github.com/earlephilhower/ESP8266SdFat/blob/9f94df41ccdb064ff4cb98d7b2737a369f6be1a0/src/FatLib/FatFile.cpp#L1514

@d-a-v
Copy link
Collaborator

d-a-v commented May 23, 2021

::availableSpaceForWrite() is in fact already used by ::availableForWrite().

I made a try and I get 0 too.

@d-a-v d-a-v added this to the 3.0.1 milestone Jun 16, 2021
@TheNitek
Copy link
Contributor Author

In Case someone needs a workaround, I just use my own StreamFile" now:

    class StreamFile: public File32 {
      int availableForWrite() override {
        return 512;
      }
    };

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 23, 2021

@TheNitek Can you try with #8167 ?

@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 Jun 24, 2021
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 a pull request may close this issue.

2 participants