This repository was archived by the owner on Jan 20, 2025. It is now read-only.
void AsyncClient::_error(err_t err) - implementation error that could lead to crashes? #94
Labels
Comments
mhightower83
added a commit
to mhightower83/ESPAsyncTCP
that referenced
this issue
Jul 16, 2019
with different core releases. Summary: The operator "new ..." was changed to "new (std::nothrow) ...", which will return NULL when the heap is out of memory. Without the change "soft WDT" was the result, starting with Arduino ESP8266 Core 2.5.0. (Note, RE:"soft WDT" - the error reporting may improve with core 2.6.) With proir core versions the library appears to work fine. ref: esp8266/Arduino#6269 (comment) To support newer lwIP versions and buffer models. All references to 1460 were replaced with TCP_MSS. If TCP_MSS is not defined (exp. 1.4v lwIP) 1460 is assumed. The ESPAsyncTCP library should build for Arduino ESP8266 Core releases: 2.3.0, 2.4.1, 2.4.2, 2.5.1, 2.5.2. It may still build with core versions 2.4.0 and 2.5.0. I did not do any regression testing with these, since they had too many issues and were quickly superseded. lwIP tcp_err() callback often resulted in crashes. The problem was a tcp_err() would come in, while processing a send or receive in the forground. The tcp_err() callback would be passed down to a client's registered disconnect CB. A common problem with SyncClient and other modules as well as some client code was: the freeing of ESPAsyncTCP AsyncClient objects via disconnect CB handlers while the library was waiting for an operstion to finished. Attempts to access bad pointers followed. For SyncClient this commonly occured during a call to delay(). On return to SyncClient _client was invalid. Also the problem described by issue me-no-dev#94 also surfaced Use of tcp_abort() required some very special handling and was very challenging to make work without changing client API. ERR_ABRT can only be used once on a return to lwIP for a given connection and since the AsyncClient structure was sometimes deleted before returning to lwIP, the state tracking became tricky. While ugly, a global variable for this seemed to work; however, I abanded it when I saw a possible reentrancy/concurrency issue. After several approaches I settled the problem by creating "class ACErrorTracker" to manage the issue. Additional Async Client considerations: The client sketch must always test if the connection is still up at loop() entry and after the return of any function call, that may have done a delay() or yield() or any ESPAsyncTCP library family call. For example, the connection could be lost during a call to _client->write(...). Client sketches that delete _client as part of their onDisconnect() handler must be very careful as _client will become invalid after calls to delay(), yield(), etc.
me-no-dev
pushed a commit
that referenced
this issue
Sep 21, 2019
* Fixes the handling tcp_err() callback and addresses build issues with different core releases. Summary: The operator "new ..." was changed to "new (std::nothrow) ...", which will return NULL when the heap is out of memory. Without the change "soft WDT" was the result, starting with Arduino ESP8266 Core 2.5.0. (Note, RE:"soft WDT" - the error reporting may improve with core 2.6.) With proir core versions the library appears to work fine. ref: esp8266/Arduino#6269 (comment) To support newer lwIP versions and buffer models. All references to 1460 were replaced with TCP_MSS. If TCP_MSS is not defined (exp. 1.4v lwIP) 1460 is assumed. The ESPAsyncTCP library should build for Arduino ESP8266 Core releases: 2.3.0, 2.4.1, 2.4.2, 2.5.1, 2.5.2. It may still build with core versions 2.4.0 and 2.5.0. I did not do any regression testing with these, since they had too many issues and were quickly superseded. lwIP tcp_err() callback often resulted in crashes. The problem was a tcp_err() would come in, while processing a send or receive in the forground. The tcp_err() callback would be passed down to a client's registered disconnect CB. A common problem with SyncClient and other modules as well as some client code was: the freeing of ESPAsyncTCP AsyncClient objects via disconnect CB handlers while the library was waiting for an operstion to finished. Attempts to access bad pointers followed. For SyncClient this commonly occured during a call to delay(). On return to SyncClient _client was invalid. Also the problem described by issue #94 also surfaced Use of tcp_abort() required some very special handling and was very challenging to make work without changing client API. ERR_ABRT can only be used once on a return to lwIP for a given connection and since the AsyncClient structure was sometimes deleted before returning to lwIP, the state tracking became tricky. While ugly, a global variable for this seemed to work; however, I abanded it when I saw a possible reentrancy/concurrency issue. After several approaches I settled the problem by creating "class ACErrorTracker" to manage the issue. Additional Async Client considerations: The client sketch must always test if the connection is still up at loop() entry and after the return of any function call, that may have done a delay() or yield() or any ESPAsyncTCP library family call. For example, the connection could be lost during a call to _client->write(...). Client sketches that delete _client as part of their onDisconnect() handler must be very careful as _client will become invalid after calls to delay(), yield(), etc. * moved DebugPrintMacros.h to the correct loccation.
|
[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
|
[STALE_DEL] This stale issue has been automatically closed. Thank you for your contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
I see that the lwip error callback is doing a few things on the _pcb data structure.
Page http://lwip.wikia.com/wiki/Raw/TCP) mentions, in bold:
So, If I undestand correctly, nothing should be done on the pcb data structure, since it might be already invalid (or will get soon to be invalid).
As such, all the code:
seems to me to be incorrect and could get you into crashes.
The only thing to be done in that callback is probably to set _pcb = NULL and call user's callback(s) if set.
The text was updated successfully, but these errors were encountered: