- 
                Notifications
    
You must be signed in to change notification settings  - Fork 23
 
Description
While looking into custom transport config for #345, I discovered that the poll_next Stream implementation for TransportContext behaves unintuitively while handling each transports' events.
litep2p/src/transport/manager/mod.rs
Lines 176 to 185 in 5e2992a
| let len = self.transports.len(); | |
| self.index = (self.index + 1) % len; | |
| for index in 0..len { | |
| let current = (self.index + index) % len; | |
| let (key, stream) = self.transports.get_index_mut(current).expect("transport to exist"); | |
| match stream.poll_next_unpin(cx) { | |
| Poll::Pending => {} | |
| Poll::Ready(None) => { | |
| return Poll::Ready(None); | |
| } | 
Examples:
In a litep2p app config with two transports (0 - tcp, 1 - ws). Debugging the current variable yields:
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1The first transport always gets skipped over on the first iteration, and we don't resume polling where we left off (self.index is sticky)
Similarly, with an app config of three transports with respective indexes (0 - tcp, 1 - ws, 2 - quic):
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1
[/litep2p/src/transport/manager/mod.rs:180:13] current = 2
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 2
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1In the case of 4 transports (0 - tcp, 1 - ws, 2 - quic, 3 - webrtc). The effects are more glaring:
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1
[/litep2p/src/transport/manager/mod.rs:180:13] current = 2
[/litep2p/src/transport/manager/mod.rs:180:13] current = 3
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 2
[/litep2p/src/transport/manager/mod.rs:180:13] current = 3
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1In this case ws is starved longer than it should be.
I think the desired order should be:
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1
[/litep2p/src/transport/manager/mod.rs:180:13] current = 2
[/litep2p/src/transport/manager/mod.rs:180:13] current = 3
[/litep2p/src/transport/manager/mod.rs:180:13] current = 0
[/litep2p/src/transport/manager/mod.rs:180:13] current = 1
[/litep2p/src/transport/manager/mod.rs:180:13] current = 2
[/litep2p/src/transport/manager/mod.rs:180:13] current = 3I'll provide a fix for this in a subsequent PR, and update the transport_events tests to validate the behavior.