Skip to content

Commit 22bae29

Browse files
Merge pull request #79 from willmmiles/error-close-stability
Error/closing stability
2 parents 15fc8ae + f736219 commit 22bae29

File tree

1 file changed

+36
-19
lines changed

1 file changed

+36
-19
lines changed

src/AsyncTCP.cpp

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ static inline lwip_tcp_event_packet_t *_get_async_event() {
253253
}
254254
}
255255

256-
static void _remove_events_for_client(AsyncClient *client) {
256+
static size_t _remove_events_for_client(AsyncClient *client) {
257257
lwip_tcp_event_packet_t *removed_event_chain;
258258
{
259259
queue_mutex_guard guard;
@@ -269,6 +269,7 @@ static void _remove_events_for_client(AsyncClient *client) {
269269
removed_event_chain = t->next;
270270
_free_event(t);
271271
}
272+
return count;
272273
};
273274

274275
void AsyncTCP_detail::handle_async_event(lwip_tcp_event_packet_t *e) {
@@ -484,7 +485,8 @@ void AsyncTCP_detail::tcp_error(void *arg, int8_t err) {
484485
// ets_printf("+E: 0x%08x\n", arg);
485486
AsyncClient *client = reinterpret_cast<AsyncClient *>(arg);
486487
if (client && client->_pcb) {
487-
_reset_tcp_callbacks(client->_pcb, client);
488+
// The pcb has already been freed by LwIP; do not attempt to clear the callbacks!
489+
_remove_events_for_client(client);
488490
client->_pcb = nullptr;
489491
}
490492

@@ -615,26 +617,35 @@ static esp_err_t _tcp_recved(tcp_pcb **pcb, size_t len) {
615617
}
616618

617619
static err_t _tcp_close_api(struct tcpip_api_call_data *api_call_msg) {
620+
// Unlike the other calls, this is not a direct wrapper of the LwIP function;
621+
// we perform the AsyncClient teardown interlocked safely with the LwIP task.
622+
623+
// As a postcondition, the queue must not have any events referencing
624+
// the AsyncClient in api_call_msg->close. This is because it is possible for
625+
// an error event to have been queued, clearing the pcb*, but after the async
626+
// thread has committed to closing/destructing the AsyncClient object.
627+
618628
tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg;
619629
msg->err = ERR_CONN;
620630
if (*msg->pcb) {
621-
// Unlike the other calls, this is not a direct wrapper of the LwIP function;
622-
// we perform the AsyncClient teardown interlocked safely with the LwIP task.
623631
tcp_pcb *pcb = *msg->pcb;
624632
_reset_tcp_callbacks(pcb, msg->close);
625-
msg->err = tcp_close(pcb);
626-
if (msg->err != ERR_OK) {
633+
if (tcp_close(pcb) != ERR_OK) {
634+
// We do not permit failure here: abandon the pcb anyways.
627635
tcp_abort(pcb);
628636
}
637+
msg->err = ERR_OK;
629638
*msg->pcb = nullptr; // PCB is now the property of LwIP
639+
} else {
640+
// Ensure there is not an error event queued for this client
641+
if (_remove_events_for_client(msg->close)) {
642+
msg->err = ERR_OK; // dispose needs to be run
643+
}
630644
}
631645
return msg->err;
632646
}
633647

634648
static esp_err_t _tcp_close(tcp_pcb **pcb, AsyncClient *client) {
635-
if (!pcb || !*pcb) {
636-
return ERR_CONN;
637-
}
638649
tcp_api_call_t msg;
639650
msg.pcb = pcb;
640651
msg.close = client;
@@ -643,21 +654,29 @@ static esp_err_t _tcp_close(tcp_pcb **pcb, AsyncClient *client) {
643654
}
644655

645656
static err_t _tcp_abort_api(struct tcpip_api_call_data *api_call_msg) {
657+
// Like close(), we must ensure that the queue is cleared
646658
tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg;
647659
msg->err = ERR_CONN;
648660
if (*msg->pcb) {
649-
tcp_abort(*msg->pcb);
661+
tcp_pcb *pcb = *msg->pcb;
662+
_reset_tcp_callbacks(pcb, msg->close);
663+
tcp_abort(pcb);
650664
*msg->pcb = nullptr; // PCB is now the property of LwIP
665+
msg->err = ERR_OK;
666+
} else {
667+
// Ensure there is not an error event queued for this client
668+
_remove_events_for_client(msg->close);
651669
}
652670
return msg->err;
653671
}
654672

655-
static esp_err_t _tcp_abort(tcp_pcb **pcb) {
673+
static esp_err_t _tcp_abort(tcp_pcb **pcb, AsyncClient *client) {
656674
if (!pcb || !*pcb) {
657675
return ERR_CONN;
658676
}
659677
tcp_api_call_t msg;
660678
msg.pcb = pcb;
679+
msg.close = client;
661680
tcpip_api_call(_tcp_abort_api, (struct tcpip_api_call_data *)&msg);
662681
return msg.err;
663682
}
@@ -903,7 +922,7 @@ void AsyncClient::close(bool now) {
903922

904923
int8_t AsyncClient::abort() {
905924
if (_pcb) {
906-
_tcp_abort(&_pcb);
925+
_tcp_abort(&_pcb, this);
907926
// _pcb is now NULL
908927
}
909928
return ERR_ABRT;
@@ -968,13 +987,11 @@ void AsyncClient::ackPacket(struct pbuf *pb) {
968987

969988
int8_t AsyncClient::_close() {
970989
// ets_printf("X: 0x%08x\n", (uint32_t)this);
971-
int8_t err = ERR_OK;
972-
if (_pcb) {
973-
_tcp_close(&_pcb, this);
974-
// _pcb is now NULL
975-
if (_discard_cb) {
976-
_discard_cb(_discard_cb_arg, this);
977-
}
990+
int8_t err = _tcp_close(&_pcb, this);
991+
// _pcb is now NULL
992+
if ((err == ERR_OK) && _discard_cb) {
993+
// _pcb was closed here
994+
_discard_cb(_discard_cb_arg, this);
978995
}
979996
return err;
980997
}

0 commit comments

Comments
 (0)