quiche: implement per-event loop processing of buffered QUIC CHLOs #8891
Conversation
Description: add UdpListenerCallbacks::onReadReady() and call quic::QuicDispatcher()::ProcessBufferedChlos() from ActiveQuicListener::onReadReady() to enable delayed opening of sessions with a per-event loop limit. Risk Level: low, not in use Testing: UdpListenerImplTest is updated to test new onReadReady() calls. EnvoyQuicDispatcherTest is updated to test that CHLOs are buffered. Docs Changes: n/a Release Notes: n/a Signed-off-by: Bence Béky <[email protected]>
|
/assign danzh2010 |
Signed-off-by: Bence Béky <[email protected]>
Signed-off-by: Bence Béky <[email protected]>
Signed-off-by: Bence Béky <[email protected]>
|
@danzh2010 The EnvoyQuicDispatcherTest tests EnvoyQuicDispatcher's behavior as far as ProcessPacket() and ProcessBufferedChlos() is concerned. However, I should also test that a new event loop calls ProcessBufferedChlos(). This has to be a ActiveQuicListenerTest. However, I do not see any packets buffered, see https://github.com/envoyproxy/envoy/pull/8891/files#diff-6a27a42427472c010c1a55f272fa72f4. Please help. |
It will start bufferring after receiving more than kNumSessionsToCreatePerEpollForTests CHLO's in one epoll loop. |
|
Is this ready for review? |
I'm still confused. The event loop is only run once in ActiveQuicListenerTest.ReceiveFullQuicCHLO. ProcessBufferedChlos() is called before any reads, when there are no CHLOs buffered. I can verify this by calling ASSERT(!quic_dispatcher_->HasChlosBuffered()); right above the ProcessBufferedChlos() call in ActiveQuicListener::onReadReady(), and the test still passes. Then QuicDispatcher should buffer the CHLO instead of processing it, and it should only be processed in the next event loop. But this is not what happens. What am I overlooking? |
|
@danzh2010 gave me direction offline. The ball is now in my court. /wait |
Signed-off-by: Bence Béky <[email protected]>
Signed-off-by: Bence Béky <[email protected]>
Signed-off-by: Bence Béky <[email protected]>
Signed-off-by: Bence Béky <[email protected]>
Finally all checks have passed, this is now ready for review. |
test/extensions/quic_listeners/quiche/envoy_quic_dispatcher_test.cc
Outdated
Show resolved
Hide resolved
test/extensions/quic_listeners/quiche/envoy_quic_dispatcher_test.cc
Outdated
Show resolved
Hide resolved
| } | ||
| ConfigureMocks(/* connection_count = */ 1); | ||
| GenerateCHLO(quic::test::TestConnectionId(1)); | ||
| dispatcher_->run(Event::Dispatcher::RunType::NonBlock); |
There was a problem hiding this comment.
Would NonBlock here cause data race where data arrives after we exit the loop? Can we still use Block and exit() instead? Actually this test maybe by nature flaky because we can't guarantee that the 16 CHLOs arrive in one event loop. Can you test with --runs_per_test to see if it flakes?
There was a problem hiding this comment.
Sorry, I do not understand the event loop well enough to be able to tell if data could possibly arrive after we exit the loop. If you think it is possible, I could use Block and exit().
I also cannot say if this test could be flaky. If you think it is possible, then we could lower the buffered CHLO limit to 1 and feed 2, that would be an improvement because 2 must have a better chance to arrive in the same event loop than 16.
There was a problem hiding this comment.
Can you try --runs_per_test to see if it's flaky first? My understanding about NonBlock is to run one loop and exit. If there is no active IO events, that's it.
Lowering the limit sounds reasonable. But again let's make sure the test doesn't flake in all builds before check in.
There was a problem hiding this comment.
Actually, I am not able to run this test locally. It crashes with segmentation fault. Both direct bazel and when using a docker container. Does the same on HEAD.
There was a problem hiding this comment.
NonBlock guarantees that the event loop will only be run once. Otherwise all CHLOs would be processed, because the limit kNumSessionsToCreatePerEpoll is per event loop. Also Block would require calling exit() from the last read_filter.onNewConnection() mock call but not from earlier ones, which is certainly doable but seems to add complexity unnecessarily to test code.
There was a problem hiding this comment.
Agree that Block and exit() doesn't satisfy our need. Are we sure that there is no race between receiving the packet and dispatcher.run() now?
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
I'm unblocked now that #8999 got merged, adding MockIoHandle that I should use in the test to mock out network sockets. |
stale
bot
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Description: add UdpListenerCallbacks::onReadReady() and call
quic::QuicDispatcher()::ProcessBufferedChlos() from
ActiveQuicListener::onReadReady() to enable delayed opening of sessions
with a per-event loop limit.
Fixes: #8756
Risk Level: low, not in use
Testing: UdpListenerImplTest is updated to test new onReadReady() calls.
EnvoyQuicDispatcherTest is updated to test that CHLOs are buffered.
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Bence Béky [email protected]