Fix for tcp_err() callbacks and other issues #115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update fork with master
Syncing with me-no-dev repo
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.
|
This issue should also be fixed: "SyncClient broken with core 2.2" #109 (comment) (I think they meant core 2.5) I also think I have PR: "Remote close() caused null reference in write" #107 (comment) covered but in a different way. I relied on local method Update1: And this one: "Error in ESPAsyncTCP after platform.io update" #102 (comment) |
|
alright :) quite the changes, but i like them :) |
mcspr
added a commit
to mcspr/espurna
that referenced
this pull request
Sep 21, 2019
- include me-no-dev/ESPAsyncTCP#115 reworked error handling multiple SyncClient fixes handle low-mem lwip builds by using correct TCP_MSS value for internal buffers - revert eb504e5 and 51703f6 - use unique_ptr for idb_client - check if idb support builds with 2.3.0 and current git
|
ESPAsyncTCP_ID305\src\ESPAsyncTCP.cpp:1324:31: error: no matching function for call to 'AsyncClient::_recv(tcp_pcb*&, pbuf*&, int)' |
mhightower83
added a commit
to mhightower83/ESPAsyncTCP
that referenced
this pull request
Oct 9, 2019
…rver::_poll(). And other missed edit for errorTracker around ASYNC_TCP_SSL_ENABLED. This should resolve @kasedy comment me-no-dev#115 (comment) and @mcspr. Tested ASYNC_TCP_SSL_ENABLED using marvinroger/async-mqtt-client/ .. examples/FullyFeaturedSSL. Ran test against test.mosquitto.org's server. Thanks to @mcspr for suggesting. Updated tcp_ssl_read() to check for fd_data being freed by callback functions. I observed this with asyncmqttclient example. When finger print did not match during fd_data->on_handshake callback, the mqtt library did a close(true) which rippled down to an tcp_ssl_free(). More improvments in debug printing to handle debug print from tcp.axtls.c.
mhightower83
added a commit
to mhightower83/ESPAsyncTCP
that referenced
this pull request
Oct 10, 2019
…rver::_poll(). And other missed edit for errorTracker around ASYNC_TCP_SSL_ENABLED. This should resolve @kasedy comment me-no-dev#115 (comment) and @mcspr. Tested ASYNC_TCP_SSL_ENABLED using marvinroger/async-mqtt-client/ .. examples/FullyFeaturedSSL. Ran test against test.mosquitto.org's server. Thanks to @mcspr for suggesting. Updated tcp_ssl_read() to check for fd_data being freed by callback functions. I observed this with asyncmqttclient example. When finger print did not match during fd_data->on_handshake callback, the mqtt library did a close(true) which rippled down to an tcp_ssl_free(). Improvements in debug printing to handle debug print from tcp.axtls.c.
kleini
pushed a commit
to kleini/ESPAsyncTCP
that referenced
this pull request
Dec 8, 2019
…rver::_poll(). And other missed edit for errorTracker around ASYNC_TCP_SSL_ENABLED. This should resolve @kasedy comment me-no-dev#115 (comment) and @mcspr. Tested ASYNC_TCP_SSL_ENABLED using marvinroger/async-mqtt-client/ .. examples/FullyFeaturedSSL. Ran test against test.mosquitto.org's server. Thanks to @mcspr for suggesting. Updated tcp_ssl_read() to check for fd_data being freed by callback functions. I observed this with asyncmqttclient example. When finger print did not match during fd_data->on_handshake callback, the mqtt library did a close(true) which rippled down to an tcp_ssl_free(). Improvements in debug printing to handle debug print from tcp.axtls.c.
jeroenst
pushed a commit
to jeroenst/ESPAsyncTCP
that referenced
this pull request
Jul 6, 2020
…rver::_poll(). And other missed edit for errorTracker around ASYNC_TCP_SSL_ENABLED. This should resolve @kasedy comment me-no-dev#115 (comment) and @mcspr. Tested ASYNC_TCP_SSL_ENABLED using marvinroger/async-mqtt-client/ .. examples/FullyFeaturedSSL. Ran test against test.mosquitto.org's server. Thanks to @mcspr for suggesting. Updated tcp_ssl_read() to check for fd_data being freed by callback functions. I observed this with asyncmqttclient example. When finger print did not match during fd_data->on_handshake callback, the mqtt library did a close(true) which rippled down to an tcp_ssl_free(). Improvements in debug printing to handle debug print from tcp.axtls.c.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
While this code has been very stable for me. I am only using ESPAsyncTCP and SyncClient my usage does not cover the other modules. However, fixes for common issues with the new (std::nothrow) operator and testing if the _client pointer had changed to NULL, were applied to all. Also in places where no testing of the results of the new operator was performed, I have added a call to panic() on NULL pointer results. This will at least provide a better hint of where things are having trouble.