diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index b60942bcd..38048fddb 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1249,7 +1249,12 @@ bool Router::dequeueDeferredLocal(DeferredLocal &out) void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src) { // Top level: handle synchronously, exactly as before the depth guard existed. - if (handleDepth == 0) { + bool nested; + { + concurrency::LockGuard g(&deferredLock); + nested = handleDepth > 0; + } + if (!nested) { handleReceived(p, src); return; } @@ -1258,8 +1263,26 @@ void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src) // handleReceived() drains it once the current dispatch unwinds, instead of stacking another // handleReceived() frame on top of the module handler (nRF52 stack overflow on config save). meshtastic_MeshPacket *copy = packetPool.allocCopy(*p); - if (copy && enqueueDeferredLocal(copy, src)) - return; + if (copy) { + // Re-check depth under the lock that also gates the drain's decrement, so a drain finishing + // while we allocated cannot leave this copy stranded in the ring. + bool stillNested = false, queued = false; + { + concurrency::LockGuard g(&deferredLock); + stillNested = handleDepth > 0; + if (stillNested) + queued = enqueueDeferredLocal(copy, src); + } + if (queued) + return; + if (!stillNested) { + // The drain finished first, so nothing would pick this up. Go through handleReceived() + // rather than dispatchReceived() so a loopback from its modules still defers. + handleReceived(copy, src); + packetPool.release(copy); + return; + } + } // Pool exhausted or queue full: drop the deferral. Leak-free and degraded but safe - the // packet still followed its normal non-loopback path (SHOULD_RELEASE, or the TX path for a @@ -1278,29 +1301,39 @@ void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src) */ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) { - handleDepth++; + { + concurrency::LockGuard g(&deferredLock); + handleDepth++; #ifdef PIO_UNIT_TESTING - if (handleDepth > maxHandleDepthObserved) - maxHandleDepthObserved = handleDepth; + if (handleDepth > maxHandleDepthObserved) + maxHandleDepthObserved = handleDepth; #endif + } dispatchReceived(p, src); - // Only the outermost frame drains. Deferred packets were produced by modules sending from - // inside dispatchReceived()'s callModules(); process them here, after the triggering frame has - // unwound, so a second handleReceived() never sits on top of a module handler. handleDepth - // stays >= 1 through the drain, so a drained packet whose own modules send more loopback - // packets enqueues them for this same loop rather than recursing: the stack stays flat and - // processing is breadth-first. - if (handleDepth == 1) { + // Decide "am I the last frame" and drop the depth in one critical section. Splitting them lets + // two frames both read the same pre-decrement value, skip the drain, and strand the ring. + for (;;) { DeferredLocal d; - while (dequeueDeferredLocal(d)) { - dispatchReceived(d.p, d.src); - packetPool.release(d.p); + { + concurrency::LockGuard g(&deferredLock); + if (handleDepth > 1) { + // Another frame is still live and will own the drain once it is last. + handleDepth--; + return; + } + if (!dequeueDeferredLocal(d)) { + // Last frame and nothing queued, so zero is reached only with the ring empty. + handleDepth--; + return; + } } + // Depth stays at 1 across the drain, so a loopback from these modules defers instead of + // recursing, and dispatch runs outside the lock. + dispatchReceived(d.p, d.src); + packetPool.release(d.p); } - - handleDepth--; } void Router::dispatchReceived(meshtastic_MeshPacket *p, RxSource src) diff --git a/src/mesh/Router.h b/src/mesh/Router.h index be686ee15..2a4d979c2 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -7,6 +7,7 @@ #include "PacketHistory.h" #include "PointerQueue.h" #include "RadioInterface.h" +#include "concurrency/LockGuard.h" #include "concurrency/OSThread.h" #include @@ -162,12 +163,8 @@ class Router : protected concurrency::OSThread, protected PacketHistory void perhapsHandleReceived(meshtastic_MeshPacket *p); /** - * Called from perhapsHandleReceived() - allows subclass message delivery behavior. - * Handle any packet that is received by an interface on this node. - * Note: some packets may merely being passed through this node and will be forwarded elsewhere. - * - * Note: this packet will never be called for messages sent/generated by this node. - * Note: this method will free the provided packet. + * Called from perhapsHandleReceived() for radio ingress and from deliverLocal() for our own + * loopback, so p may be locally generated. Does NOT free p; the caller still owns it. */ void handleReceived(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO); @@ -192,6 +189,10 @@ class Router : protected concurrency::OSThread, protected PacketHistory /// so a locally-sent loopback packet must be deferred rather than handled synchronously. uint8_t handleDepth = 0; + /// Guards handleDepth and the deferred ring below. nRF52 drives the router from the BLE task as + /// well as the loop task, so both are read-modify-written from two tasks. + concurrency::Lock deferredLock; + /// A local loopback packet whose handleReceived() was deferred because it was produced from /// inside callModules(). The queue owns the packet; its RxSource travels with it so the drain /// dispatches it with the origin the sender intended (RX_SRC_LOCAL stays local). @@ -210,8 +211,10 @@ class Router : protected concurrency::OSThread, protected PacketHistory uint8_t deferredLocalCount = 0; // entries currently queued /// Queue a deferred local packet. Returns false (and queues nothing) when full. + /// Caller must hold deferredLock: the enqueue decision is atomic with the drain's depth update. bool enqueueDeferredLocal(meshtastic_MeshPacket *p, RxSource src); /// Pop the oldest deferred local packet into out. Returns false when empty. + /// Caller must hold deferredLock. bool dequeueDeferredLocal(DeferredLocal &out); /** Frees the provided packet, and generates a NAK indicating the specifed error while sending */