From b1e41be02fbb8ca79d5eda61248b281f75b00ee6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 10 Mar 2026 14:46:02 -1000 Subject: [PATCH 1/2] clear error first --- esphome/components/socket/lwip_raw_tcp_impl.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 799b09e844d..86a04f52cf6 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -664,9 +664,12 @@ ssize_t LWIPRawImpl::writev(const struct iovec *iov, int iovcnt) { LWIPRawListenImpl::~LWIPRawListenImpl() { LWIP_LOCK(); - // Abort any queued PCBs that were never accepted by the main loop + // Abort any queued PCBs that were never accepted by the main loop. + // Clear the error callback first — tcp_abort triggers it, and we don't + // want s_accepted_pcb_err_fn writing to slots during destruction. for (uint8_t i = 0; i < this->accepted_socket_count_; i++) { if (this->accepted_pcbs_[i] != nullptr) { + tcp_err(this->accepted_pcbs_[i], nullptr); tcp_abort(this->accepted_pcbs_[i]); this->accepted_pcbs_[i] = nullptr; } From a9e921e0530b3ed28d895aa67548f44498154189 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 10 Mar 2026 14:52:21 -1000 Subject: [PATCH 2/2] [socket] Refactor accept() to skip null entries without duplicate code Consolidate the dequeue + shift + tcp_arg update into a single while loop that skips null entries (freed by lwip while queued) and returns the first valid PCB. Eliminates the duplicated shift/update logic. --- .../components/socket/lwip_raw_tcp_impl.cpp | 76 +++++++++---------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 20dcd7d4424..2c2c42179df 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -748,48 +748,46 @@ std::unique_ptr LWIPRawListenImpl::accept(struct sockaddr *addr, so errno = EBADF; return nullptr; } - if (this->accepted_socket_count_ == 0) { - errno = EWOULDBLOCK; - return nullptr; - } - // Take entry from front of queue - QueuedPcb entry = this->accepted_pcbs_[0]; - // Shift remaining entries forward - for (uint8_t i = 1; i < this->accepted_socket_count_; i++) { - this->accepted_pcbs_[i - 1] = this->accepted_pcbs_[i]; - } - this->accepted_pcbs_[this->accepted_socket_count_ - 1] = {}; - this->accepted_socket_count_--; - // Update tcp_arg for remaining queued PCBs — their array slots shifted by one. - // Safe because we hold LWIP_LOCK, so err/recv callbacks can't fire during the update. - for (uint8_t i = 0; i < this->accepted_socket_count_; i++) { - if (this->accepted_pcbs_[i].pcb != nullptr) { - tcp_arg(this->accepted_pcbs_[i].pcb, &this->accepted_pcbs_[i]); + // Dequeue front entry, skipping any null entries (PCBs freed by lwip while queued). + // The error callback nulled their pcb pointers; clean up buffered data and discard. + while (this->accepted_socket_count_ > 0) { + QueuedPcb entry = this->accepted_pcbs_[0]; + // Shift remaining entries forward and update tcp_arg pointers (slots shifted by one). + // Safe because we hold LWIP_LOCK, so err/recv callbacks can't fire during the update. + for (uint8_t i = 1; i < this->accepted_socket_count_; i++) { + this->accepted_pcbs_[i - 1] = this->accepted_pcbs_[i]; } - } - LWIP_LOG("Connection accepted by application, queue size: %d", this->accepted_socket_count_); - if (entry.pcb == nullptr) { - // PCB was freed by lwip (RST/timeout) while queued — the temporary error callback - // nulled our pointer. Free any buffered data and return EWOULDBLOCK. - if (entry.rx_buf != nullptr) { - pbuf_free(entry.rx_buf); + this->accepted_pcbs_[this->accepted_socket_count_ - 1] = {}; + this->accepted_socket_count_--; + for (uint8_t i = 0; i < this->accepted_socket_count_; i++) { + if (this->accepted_pcbs_[i].pcb != nullptr) { + tcp_arg(this->accepted_pcbs_[i].pcb, &this->accepted_pcbs_[i]); + } } - errno = EWOULDBLOCK; - return nullptr; + if (entry.pcb == nullptr) { + // PCB was freed by lwip (RST/timeout) while queued — discard and try next + if (entry.rx_buf != nullptr) { + pbuf_free(entry.rx_buf); + } + continue; + } + LWIP_LOG("Connection accepted by application, queue size: %d", this->accepted_socket_count_); + // Create socket wrapper on the main loop (not in accept callback) to avoid + // heap allocation in IRQ context on RP2040. Transfer any data received while queued. + auto sock = make_unique(this->family_, entry.pcb); + sock->init(entry.rx_buf); + if (entry.rx_closed) { + // Remote closed while queued — mark so read() returns EOF after buffered data + sock->rx_closed_ = true; + } + if (addr != nullptr) { + sock->getpeername(addr, addrlen); + } + LWIP_LOG("accept(%p)", sock.get()); + return sock; } - // Create socket wrapper on the main loop (not in accept callback) to avoid - // heap allocation in IRQ context on RP2040. Transfer any data received while queued. - auto sock = make_unique(this->family_, entry.pcb); - sock->init(entry.rx_buf); - if (entry.rx_closed) { - // Remote closed while queued — mark so read() returns EOF after buffered data - sock->rx_closed_ = true; - } - if (addr != nullptr) { - sock->getpeername(addr, addrlen); - } - LWIP_LOG("accept(%p)", sock.get()); - return sock; + errno = EWOULDBLOCK; + return nullptr; } int LWIPRawListenImpl::listen(int backlog) {