[BUG] beginChunkedResponse

[Levak]

Description

I upgraded ESPAsyncWebServer from v3.1.0 to the latest v3.3.x and found out that beginChunkedResponse behaved differently. I am unsure what causes the exact issue, but the result is that my request acts like there is no file in the directory I am traversing.

I managed to git-bisect by hand and find the problematic commit: 571eac4

Before that commit, I can traverse File root for files inside. After that commit, I never enter the while loop.

Here is a simplified view of the function:

fs::FS &fileSystem = SD_MMC;

void handleList(AsyncWebServerRequest *request) {
  File root = fileSystem.open("/");
  String txt;
  bool finished;

  AsyncWebServerResponse *response = request->beginChunkedResponse(
    "application/json",
    [root, txt, finished]
      (uint8_t* buffer, const size_t max_len, const size_t index) mutable
      -> size_t
  {
    log_i("index %u, %u", index, max_len);
    if (index == 0) { // <========================= Reached first (index 0)
      txt = "[";
      finished = false;
    }

    while (File file = root.openNextFile()) {  // <==== never enters here
      log_i("file %s", file.name());
      txt += "\"" + file.name() + "\"",;

      if (txt.length() > max_len) {
        memcpy(buffer, txt.c_str(), max_len);
        txt = txt.substring(max_len);
        log_i("return %u", max_len);
        return max_len;
      }
    }

    if (!finished) {
      finished = true;
      txt += "]";
      memcpy(buffer, txt.c_str(), txt.length());
      root.close();
      log_i("return %u", txt.length());
      return txt.length();
    } else { // <============================== Reached second (index 2)
      log_i("return %u", 0);
      return 0;
    }
  });

  request->send(response);
}

Result:

[]

Result in logs:

[ 34877][I][sdwifi.ino:682] operator()(): index 0, 5511
[ 34884][I][sdwifi.ino:728] operator()(): return 2
[ 35051][I][sdwifi.ino:682] operator()(): index 2, 5501
[ 35056][I][sdwifi.ino:731] operator()(): return 0

While when using commit 741841a it works and here is the output in logs:

[ 37648][I][sdwifi.ino:674] operator()(): index 0, 5511
[ 38298][I][sdwifi.ino:682] operator()(): file Lamp_Shade_1_PLA_1h17m.gcode
[ 38324][I][sdwifi.ino:720] operator()(): return 32
[ 38338][I][sdwifi.ino:674] operator()(): index 32, 5471
[ 38344][I][sdwifi.ino:723] operator()(): return 0

Full source code: Link to the source code where the issue happens

Board: esp32-pico-d4 (SD WIFI PRO)

Considering this is a dual-core ESP32, I did try to set CONFIG_ASYNC_TCP_RUNNING_CORE to 0 or 1 with no success.

[Levak]

Reduced the scope to the changes made to AsyncWebServerRequest::send:

 void AsyncWebServerRequest::send(AsyncWebServerResponse* response) {
+  if (_sent)
+    return;
+  if (_response)
+    delete _response;
   _response = response;
   if (_response == NULL) {
     _client->close(true);
@@ -716,15 +730,8 @@ void AsyncWebServerRequest::send(AsyncWebServerResponse* response) {
     _sent = true;
     return;
   }
-  if (!_response->_sourceValid()) {
-    delete response;
-    _response = NULL;
+  if (!_response->_sourceValid())
     send(500);
-  } else {
-    _client->setRxTimeout(0);
-    _response->_respond(this);
-    _sent = true;
-  }
 }

If I keep the red(-) part, my code works. If I use the green(+) part, my code yields no files. As if my File was closed by a destructor.

[mathieucarbou]

thanks for the report! I appreciate the work and time spent in your findings which helps me answer more precisely about what is happening.

The diff you are talking to is related to the introduction of middlewares.

Before, calling send() would commit the response on the wire (_response->_respond(this);), so your variables below are still in scope when calling send().

  File root = fileSystem.open("/");
  String txt;
  bool finished;

Now, send() just "registers" the response, in order to let next middlewares interact with its headers, or even eventually completely replace it. So the response object is leaving the scope of your handler and its variables get destroyed (root, txt and finished).

The callback of beginChunkedResponse can be recalled until completion, and it captures these variables by copy.

The correct way of doing what you want to do is to use the request->_tempObject pointer to hold the states. Right now, I suspect that the way you have coded the function was hiding the fact that your variables going out of scope could have an impact on your code.

Something like that would ensure that your request object becomes the owner of the states:

struct States {
      File root;
      String txt;
      bool finished;
    };

  server.on("/chunk", HTTP_GET, [](AsyncWebServerRequest* request) {    
    States* states = new States();
    states.root = ...
    states.txt = ...
    states.finished = ...
    
    request->_tempObject = states;

    AsyncWebServerResponse* response = request->beginChunkedResponse("text/html", [request](uint8_t* buffer, size_t maxLen, size_t index) -> size_t {
      States* states = (States*) request->_tempObject;
      // ...
    });
    request->send(response);
  });

You don't even need to free the _tempObject when finished: ESPAsyncWebServer does that fro you when the request tis destroyed, as a safety measure in case your callback is not called to handle the "finished" state

[Levak]

Hi !
Thank you for your detailed answer, I did suspect I was doing something wrong, just clueless why such change would matter.

Nevertheless, I did try to implement your solution, and I'm afraid it's still doing the same.

struct HandleListStates {
  File root;
  String txt;
  bool finished;
};

void handleList(AsyncWebServerRequest *request)
{
    struct HandleListStates* states = new HandleListStates();
    states->root = fileSystem.open("/");
    request->_tempObject = states;

    AsyncWebServerResponse *response = request->beginChunkedResponse(
      "application/json",
      [request]
      (uint8_t* buffer, const size_t max_len, const size_t index) mutable -> size_t
    {
      struct HandleListStates* states = (struct HandleListStates*) request->_tempObject;
      [...]
      while (File file = states->root.openNextFile()) { // still not entering the loop
        [...]
      }
      [...]
    });

    request->send(response);
}

Any idea ?

Regards,

[mathieucarbou]

Not on my mind... I will have to check deeper because this is supposed to work. What you did here is right.

I will add an example in the SimpleServer code and get back to you so that you can complete this example in case I am not able to reproduce.

[Levak]

I tried to move the filesystem.open() inside the chunk handler, I get this in the logs:

[ 65343][E][vfs_api.cpp:23] open(): File system is not mounted

Which should indicate us something is wrong with the context.
Or is it still the same issue as loosing the variable out of scope?

          [...]
          if (index == 0) {
            states->root = fileSystem.open(states->path);
            [...]

[mathieucarbou]

Or is it still the same issue as loosing the variable out of scope?

No since the states are within the request object, which stays the same and the callback can be called multiple times until completion...

File system is not mounted is weird though...

Do you have the ability to test with LittleFS instead of SD_MMC ? That's what I was goign to test anyway in the simple example...

[Levak]

I am so sorry, I just understood what went wrong.
In this project, we mount and unmount the SD card only when necessary.
Since we go out of context, we unmount the SD, so when the handler triggers, we already have the SD unmounted.

Now it works 🎉

So, in the end, I did need your solution, but I also needed to not unmount the SD card at the end of the handler ^^

There is no bug there, assuming we use a context saved in _tempObject. I'd recon adding a small example would help the next person that encounters this ^^.

[mathieucarbou]

Awesome!

So, in the end, I did need your solution, but I also needed to not unmount the SD card at the end of the handler ^^

Ahah! yes, that's right! what you can do though is unmount from within the callback in the if(finished) block because you know that the callback won't be called after and the response is then fully sent.

There is no bug there, assuming we use a context saved in _tempObject. I'd recon adding a small example would help the next person that encounters this ^^.

This thing comes from the original repo and was a little bit documented in the original doc:

https://github.com/mathieucarbou/ESPAsyncWebServer?tab=readme-ov-file#body-data-handling

I will move this into a discussion for reference purposes. This was an interesting use case ;-)