Add EventLoopIteration to Connection methods: send_on_path, recv and on_timeout#2410
Add EventLoopIteration to Connection methods: send_on_path, recv and on_timeout#2410antoniovicente wants to merge 4 commits into
Conversation
This change simplifies the event loop and removes the need to bias the select! to avoid starvation by serializing all the event loop actions and clarifying the start and end of the work loop iteration.
antoniovicente
force-pushed
the
antonio/event_loop_iter
branch
4 times, most recently
from
c0a7b0f to
0898578
Compare
antoniovicente
force-pushed
the
antonio/event_loop_iter
branch
from
0898578 to
19a80e0
Compare
| } | ||
| } | ||
|
|
||
| #[no_mangle] |
There was a problem hiding this comment.
general comment: this seems like a sizeable amount of code to add to the FFI and it doesn't update the C examples to call it. I'd suggest punting on C entirely for now
There was a problem hiding this comment.
I think that the right thing would be to update examples to use the new API and fully delete the old API in a future PR.
| /// [`on_timeout`]: Connection::on_timeout | ||
| pub struct EventLoopIteration { | ||
| /// The time at which this event loop iteration started. | ||
| now: Instant, |
There was a problem hiding this comment.
nit: if the field represents the start, just call it that
| now: Instant, | |
| start: Instant, |
There was a problem hiding this comment.
Excellent suggestion. ty.
| /// ``` | ||
| pub fn recv(&mut self, buf: &mut [u8], info: RecvInfo) -> Result<usize> { | ||
| pub fn recv( | ||
| &mut self, iteration: &EventLoopIteration, buf: &mut [u8], info: RecvInfo, |
There was a problem hiding this comment.
I think the changes to extant methods are too disruptive TBH, as it pushes work on all downstreams and risks them making the wrong decision. If its safe to call them with a new EventLoopIteration each time, that should just be the default internal behaviour.
IOW like with the FFI proposal, I'd suggest adding variants that have a suffix of with_iteration, _with_ev_loop, or something. Then the old methods can just construct a EventLoopIteration to pass onto the new ones
There was a problem hiding this comment.
It is not correct to pass a new EventLoopIteration each time. Currently it mostly works, but #2328 require this information and it does need to be correct.
Creating a new EventLoopIteration each time will result in BBR not working correctly; app-limited will be computed incorrectly some of the time.
There was a problem hiding this comment.
Right but requiring all apps to make a change for a congestion control algo they might not be interested in seems pretty extreme.
As this PR highlights, we have dozens of tests that do not care about the app-limited behaviour one way or another
There was a problem hiding this comment.
If the solution is to have different API requirements for BBR and not, I think we could do that. But I'ld like us to make a decision.
We could do something to require that the hook introduced in #2328 be called if the code wants to use BBR, or we could try forcing a wider API change to force users of quiche to reconsider the correctness of their integrations.
Ideally we'ld have a simpler API to the event loop which provides hooks to consume read packets, emit packets and handling of timeouts using a single call, instead of the current API which forces the app to do timeout handling, recv and send in the right order.
| ) -> QuicResult<()> { | ||
| std::future::poll_fn(|_| { | ||
| match self.gather_data_from_quiche_conn(qconn, buffer) { | ||
| match self.gather_data_from_quiche_conn(iteration, qconn, buffer) { |
There was a problem hiding this comment.
TODO: We may need a new iteration on every poll so we can have a fresh now
| } | ||
| } | ||
|
|
||
| #[no_mangle] |
There was a problem hiding this comment.
I think that the right thing would be to update examples to use the new API and fully delete the old API in a future PR.
| /// ``` | ||
| pub fn recv(&mut self, buf: &mut [u8], info: RecvInfo) -> Result<usize> { | ||
| pub fn recv( | ||
| &mut self, iteration: &EventLoopIteration, buf: &mut [u8], info: RecvInfo, |
There was a problem hiding this comment.
It is not correct to pass a new EventLoopIteration each time. Currently it mostly works, but #2328 require this information and it does need to be correct.
Creating a new EventLoopIteration each time will result in BBR not working correctly; app-limited will be computed incorrectly some of the time.
| /// [`on_timeout`]: Connection::on_timeout | ||
| pub struct EventLoopIteration { | ||
| /// The time at which this event loop iteration started. | ||
| now: Instant, |
There was a problem hiding this comment.
Excellent suggestion. ty.
|
|
||
| loop { | ||
| let now = Instant::now(); | ||
| let iteration = EventLoopIteration::new(); |
There was a problem hiding this comment.
I'm struggling to follow this change a bit since I'm less familiar with TQ guts. It would seem that if ever we broke the loop at https://github.com/cloudflare/quiche/pull/2410/changes#diff-77397d0e30b86551f31a2fc4fc5e2adc5fb62fd3091c5e8a35b950bd43fc7c1cR345, then the quiche timeout wouldn't be called. Is that ok?
2 participants
This refactor is in preparation for #2328