Bugfix for low-priority packet replacement when TX queue is full #5827
Conversation
|
Somehow 
|
fifieldt
force-pushed
the
priority-drop-fix
branch
from
e7a93d2 to
9f9758c
Compare
| @@ -139,8 +147,11 @@ bool MeshPacketQueue::replaceLowerPriorityPacket(meshtastic_MeshPacket *p) | |||
| for (; refPacket->tx_after && it != queue.begin(); refPacket = *--it) | |||
| ; | |||
| if (!refPacket->tx_after && refPacket->priority < p->priority) { | |||
| LOG_WARN("Dropping non-late packet 0x%08x to make room in the TX queue for higher-priority packet 0x%08x", | |||
| refPacket->id, p->id); | |||
| packetPool.release(refPacket); | |||
There was a problem hiding this comment.
There is no call to erase the refPacket from the queue. The queue size doesn't decrease. If the queue was full, this will loop back in to replace the lower priority packet again.
There was a problem hiding this comment.
Thank you. That explains the loop we were seeing earlier 🙂.
I should have spotted that the first time around. Apologies for missing it!
There was a problem hiding this comment.
No worries! It took me a few times to spot it as well. I kept assuming packetPool.release was taking care of removing it.
| @@ -124,9 +131,10 @@ bool MeshPacketQueue::replaceLowerPriorityPacket(meshtastic_MeshPacket *p) | |||
| // Check if the packet at the back has a lower priority than the new packet | |||
| auto &backPacket = queue.back(); | |||
There was a problem hiding this comment.
I suggest removing the reference here. Maybe auto *backPacket to make it clearer what the type is.
Calling pop_back while holding a reference might not always be safe.
caveman99
force-pushed
the
priority-drop-fix
branch
from
9f9758c to
3be455a
Compare
Add the intended replacement packet (not the packet that was just removed, which was clearly a typo) to the TX queue after dropping a lower-priority packet. Fixes a use-after-free.
Also corrects the function comment, and adds some debug logging when packets are dropped due to a full TX queue.
Might resolve #5826; testing on that by @Talie5in is pending.