Flush stream after we try to send data - always. #8405
Conversation
eskimor
added
A0-please_review
Co-authored-by: Bastian Köcher <[email protected]>
| if let State::Open { notifications_sink_rx, out_substream: out_substream @ Some(_), .. } | ||
| = &mut self.protocols[protocol_index].state | ||
| { | ||
| let mut flush_needed = false; |
There was a problem hiding this comment.
Since this is local, if a flush is needed but poll_flush returns Poll::Pending, it looks like the handler may not call poll_flush() again when it is next polled? I would think that not wanting to track such state is why the current code always calls poll_flush() for each open substream in the removed block above. Please correct me if I'm mistaken.
There was a problem hiding this comment.
Good point!
The problem that we have encountered is that flush wasn't called at all, we assume that this was probably due to other substreams being more busy or whatever.
As I'm thinking more about this, the current implementation isn't really robust. It could happen that a stream with a lower procotol_index could starve other streams. There is already this events_queue. We should insert every event in there and to ensure that we poll all incoming and outgoing streams. At the end of this loop we should check if there is something in the events_queue and return that.
There was a problem hiding this comment.
As I'm thinking more about this, the current implementation isn't really robust. It could happen that a stream with a lower
procotol_indexcould starve other streams. There is already thisevents_queue. We should insert every event in there and to ensure that we poll all incoming and outgoing streams. At the end of this loop we should check if there is something in theevents_queueand return that.
Problem: We lose back pressure. events_queue could grow indefinitely in that case.
There was a problem hiding this comment.
Why do we loose the back pressure? The event queue was checked before we polled for new events?
There was a problem hiding this comment.
Yeah, I wanted to avoid checking before and after and if we only check afterwards, we got that issue. If we only checked first, we would return Pending although we are actually Ready. Either way @tomaka seems skeptic that we solve anything here.
|
Hopefully better fix here. |
No description provided.