L2capChannel::accept stuck forever when there is a disconnection event in runner

[jancespivo]

During L2capChannel::accept

runner.run() logged:

INFO [host] disconnection event on handle 64, reason: Remote User Terminated Connection

And L2capChannel::accept doesn't return any result. Just stuck in unknown state forever.

IMO this disconnection event should cause L2capChannel::accept to return an error.

I've tried to find the way how to fix it myself and I found this line

trouble/host/src/channel_manager.rs

Line 203 in ac8c26d

_ => {}

chan.state is ChannelState::Disconnected which is starting state so I don't know how to get from there.

Thanks!

[lulf]

Thanks for raising the issue. Indeed it's a bit hard to fix because when a disconnect event happens from the controller, the host will mark the channels for that connected as disconnected, which makes it indistinguishable from the default state. The problem is that even if we introduce another state that 'ControllerDisconnected', it needs something that is for sure invoked (i.e. not application code) to move it into the fully Disconnected state. It cannot do that inside the accept(), because if the application drops the future, you get a resource leak.

There are a few ways to fix this I think:

1. Use a timeout with your accept(), like this (this is what we do at our company):

    let Ok(result) = with_timeout(Duration::from_secs(15), L2capChannel::accept(...)).await else {
        return Err(...)
    }

2. Add some other 'garbage collection' task to the host controller that moves the channels from the 'ControllerDisconnected' to the 'Disconnected' state after some time. This assumes the accept() gets to poll the state before it is moved.

3. Add some unique 'acceptor handle' which can be signaled by the host runner.

I think the third option is the most reliable but also more complex because now you need to deal with that resource as well. I think for most use cases, option 1. will work fine. But it should probably be better documented, used in the examples etc.

[jancespivo]

@lulf Thanks. Fully agree. See my draft PR as well where I wrote my early findings.

Actually I thought in the same direction.

1. Workaround with adding timeouts everywhere. Basically adding embassy_time::with_timeout everywhere. This is the easiest and the safest way as well for now. Go to solution as the first step definitely. And maybe the final. 👍
2. This could be hard. I played with adding new states and it never worked properly on the host or on remote side. Always there were some race conditions and it was failing (read: waiting forever) somewhere. Depending when some future was polled. It can work, however finally it could introduce many subtle bugs anywhere in the future.
3. Some events based deterministic signaling from the host runner is definitely the proper solution. But I don't think it is worth it. See reasoning below.

Disconnecting, RF noise, current peaks, and bugs naturally occurs. So from my point of view, just adding with_timeout everywhere is the way, because it is needed anyway even for 2. and 3.

The only design choice I would do is whether to incorporate timeouts to all functions by default (even indefinitely as a default). There are some kinds of timeouts in the code already. I don't have a strong opinion about it.

Thank you again!

[lulf]

The only design choice I would do is whether to incorporate timeouts to all functions by default (even indefinitely as a default). There are some kinds of timeouts in the code already. I don't have a strong opinion about it.

Yes, I think maybe that's the way to go. That way at least you'd have to explicitly set timeout = 0 to get today's behavior. Feel free to raise a PR for that if you'd like.