Skip to content

Fix inverted notify/notify_all in BQueue.Interrupt - #177

Open
winklemad wants to merge 1 commit into
unitreerobotics:masterfrom
winklemad:fix/bqueue-interrupt-notify-inverted
Open

Fix inverted notify/notify_all in BQueue.Interrupt#177
winklemad wants to merge 1 commit into
unitreerobotics:masterfrom
winklemad:fix/bqueue-interrupt-notify-inverted

Conversation

@winklemad

Copy link
Copy Markdown

Problem

BQueue.Interrupt(notifyAll) calls the wrong Condition method for each branch:

def Interrupt(self, notifyAll: bool = False):
    with self.__condition:
        if notifyAll:
            self.__condition.notify()        # wakes only ONE waiter
        else:
            self.__condition.notify_all()    # wakes ALL waiters

The two branches are swapped relative to the parameter's meaning: notifyAll=True wakes only a single blocked Get() waiter, while the default notifyAll=False wakes all of them. So a caller that relies on Interrupt(notifyAll=True) to release every consumer blocked in Get() only releases one; the rest keep waiting until their timeout (or forever, if called with timeout=None).

The current in-tree caller (core/channel.py Close()) uses the default and has a single consumer thread, so it happens to work — but the method's contract is still inverted.

Reproduce

import threading, time
from unitree_sdk2py.utils.bqueue import BQueue

q = BQueue(maxLen=10)
woke = []
def consumer(i):
    q.Get(timeout=5.0)   # blocks: queue is empty
    woke.append(i)

for i in range(2):
    threading.Thread(target=consumer, args=(i,), daemon=True).start()
time.sleep(0.3)          # both threads are now blocked in Get()

q.Interrupt(notifyAll=True)   # intent: wake BOTH consumers
time.sleep(0.3)
print(sorted(woke))      # [0]  -> only one woke; expected [0, 1]

Fix

Swap the two branches so notifyAll=True calls notify_all() and notifyAll=False calls notify(). After the fix the snippet prints [0, 1], and the single-consumer Interrupt() path used by channel.py is unchanged.

BQueue.Interrupt(notifyAll) called notify() when notifyAll was True and
notify_all() when it was False — the opposite of the parameter's meaning.
As a result, Interrupt(notifyAll=True) woke only a single blocked Get()
waiter instead of all of them. Swap the two branches so the parameter
behaves as documented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant