ControllerState.waker is never signaled

[sidit77]

Hi,

I'm currently through this project to get some inspiration on how to improve my own Bluetooth stack and I think I've stumbled on a small bug.

Currently the implementation of the ControllerState looks like this:

struct ControllerState<const SLOTS: usize> {
    ...
    waker: AtomicWaker,
}

impl<const SLOTS: usize> ControllerState<SLOTS> {
    async fn acquire(&self, op: cmd::Opcode, event: *mut [u8]) -> (&Signal<NoopRawMutex, CommandResponse>, usize) {
        ...
        poll_fn(|cx| match self.acquire_slot(op, event) {
            Some(ret) => Poll::Ready(ret),
            None => {
                self.waker.register(cx.waker()); // <-- Waker is registered here if no viable slot is available
                Poll::Pending
            }
        })
        .await
    }
}

However, the implementation never calls self.waker.wake(). Unless I'm mistaken, this should cause the ControllerState::acquire to potentially stall indefinitely.

I think the correct action is to add a self.waker.wake() call to ControllerState::release_slot.

[lulf]

Hi,

I'm currently through this project to get some inspiration on how to improve my own Bluetooth stack and I think I've stumbled on a small bug.

Currently the implementation of the ControllerState looks like this:

struct ControllerState {
...
waker: AtomicWaker,
}

impl ControllerState {
async fn acquire(&self, op: cmd::Opcode, event: *mut [u8]) -> (&Signal<NoopRawMutex, CommandResponse>, usize) {
...
poll_fn(|cx| match self.acquire_slot(op, event) {
Some(ret) => Poll::Ready(ret),
None => {
self.waker.register(cx.waker()); // <-- Waker is registered here if no viable slot is available
Poll::Pending
}
})
.await
}
}

However, the implementation never calls self.waker.wake(). Unless I'm mistaken, this should cause the ControllerState::acquire to potentially stall indefinitely.

I think the correct action is to add a self.waker.wake() call to ControllerState::release_slot.

I believe this is the correct fix also. This could cause all sorts of issues I suppose, but perhaps most users have used enough slots for this not state to not occur.

[sidit77]

I think the code might also be somewhat broken when you have more than one task waiting for a slot, as the second waker will override the first.
But I'm honestly not sure how many people are actually waiting for more than one command at the same time.

I feel like most people will write something like this:

ctl.exec(...).await?;
ctl.exec(...).await?;

rather than

let cmd1 = ctl.exec(...);
let cmd2 = ctl.exec(...);
let (ret1, ret2) = join2(cmd1, cmd2).await;
ret1?;
ret2?;

I’m not sure. When I first built a host stack, I tried implementing concurrency at every layer, but it quickly became a mess. That’s why I’m now looking at how others have approached it.

I don't envy you for trying to make something that goes beyond a hacky weekend project.

[lulf]

I think the code might also be somewhat broken when you have more than one task waiting for a slot, as the second waker will override the first. But I'm honestly not sure how many people are actually waiting for more than one command at the same time.

I feel like most people will write something like this:

ctl.exec(...).await?;
ctl.exec(...).await?;

rather than

let cmd1 = ctl.exec(...);
let cmd2 = ctl.exec(...);
let (ret1, ret2) = join2(cmd1, cmd2).await;
ret1?;
ret2?;

I’m not sure. When I first built a host stack, I tried implementing concurrency at every layer, but it quickly became a mess. That’s why I’m now looking at how others have approached it.

I don't envy you for trying to make something that goes beyond a hacky weekend project.

That's true, we could use the MultiWaker or some other intrinsic waker from maitake-sync perhaps, I'm not sure if it's worth the overhead.