Fix server timeout caused by a packet loss at Handshake #66
Conversation
junhochoi
changed the title
junhochoi
changed the title
|
BTW this build failure is due to a change in rustfmt. Not that it matter too much, but we can squelch it with a a 1-liner 591feb4 |
There was a problem hiding this comment.
The fundamental problem here is that we process STREAM frames as well as allow stream writes (including creating stream state) before the handshake is complete, which is probably wrong, so maybe we should just prevent that instead?
In any case we need a test for this case, can you look into adding one @junhochoi ?
|
Also, I fixed the rustfmt config in master, so please rebase to resolve the build failures. |
That was one of my initial thoughts but I figured it was by done on purpose to avoid blocking request processing while waiting for Handshake retransmits. As I get more familiar with this part of the library, it seems weirdly inconsistent that we would start doing things like request processing, while simultaneously delaying the read of the peer's transport params until handshake_complete. |
junhochoi
force-pushed
the
269.fix_timeout
branch
from
6fe07a4
to
265924c
Compare
When there is a Handshake packet loss, depending on the state server or client still can proceed and send new data. e.g. Client: send Initial #0 Server: got Initial, send ACK, send Initial #0 Client: send Handshake #0, #1 Server: got Hanshake #0, #1, send ACK, send Handshake #0 Client: got Handshake #0, send ACK [#2] #1 [#1 lost], send Application data (h3 request) #0 (Application data is tranferred in Stream ID 0) Server: got Application #0 At this point, Handshake is not complete, but both parties received H3 request and Server writes response header and body to Stream send buffer. However when Stream ID 0 is created at Server, Handshake is not complete yet, so it's created with stream.send.max_data = 0 because default value of initial_max_stream_data_bidi_local is 0 until we get transport parameter. This leads response data (header and body) is queued but not sent because of max_data is not updated for opened stream. To solve this issue, when handshake is complete, scan opened stream and update send.max_data by calling send.update_max_data().
Co-Authored-By: Lucas Pardue <lucaspardue.24.7@gmail.com>
When calling stream.send.update_max_data(), default value need to be what we use when Stream.SendBuf is created.
junhochoi
force-pushed
the
269.fix_timeout
branch
from
265924c
to
0828e9f
Compare
In this case s.send.get_max_data() is always 0 because streams opened at this point have default values (0), not from a transport parameter.
While looking at it I thought the similar thing but for now I think it is ok/right to delay stream write until handshake is done. On the other hand we may reject stream creation until handshake is complete but it may mean a packet loss from peer side, causing retransmit. |
Test will try to make a ACK from client at the end of Handshake lost, check if stream#0.send.get_max_data() == 0, send pending ACK again, and check if stream#0.send.get_max_data() > 0 to simulate the case. testing::recv_send() is split into recv() and send()
|
@LPardue yes, this was originally intended to allow support for receiving 0-RTT data though it hasn't been implemented yet. But note that applications can already enforce not reading data (e.g. process HTTP requests) before connection is established (and indeed we do in our applications), but the problem is that STREAM frames are still processed by the library, so stream state is initialized anyway. @junhochoi I'm thinking maybe we could still process (decrypt and parse) a limited number of short header ("Application") packets before handshake is complete, but instead of processing the frames immediately they could be buffered and processed when the handshake is completed. This way we wouldn't need to drop packets, unless we receive more than a certain amount (5? 10? To avoid potential DoS). |
|
I made 188588c which should make it easier to implement the frame buffering I suggested in my previous comment, if someone wants to try. |
Sounds like a good idea. e.g. client is doing POST with a big file. up to initcwnd (10?) will be acceptable. I think QUIC transport draft has something similar. |
|
I looked into implementing the frame buffering thing and made #68. Note that this still allows applications to send data before the handshake is completed, but that can be implemnted separately (and shouldn't be a problem right now for the HTTP/3 use-case). |
Basically porting #68 changes into this branch. Add early_app_pkts to count the number of packet before Handshake complete.
Thanks. I think the major difference here is whether to allow Application read the packets before Handshake completes -- #66 branch allows and #68 branch doesn't allow (instead it buffers packets). Good thing of #66 is during recovering lost Handshake packet server can read request stream, while #68 waits until Handshake completes and let Application read packets afterwards. (so #66 can be a little faster when origin latency is high in this case but not impacing when cached or directly served by this server). Another aspect is whether to allow Application read packets (stream data) before Handshake fully completes can be a security concern. I think this can be acceptable because until Handshake fully completes server cannot send response back (write is blocked) and if nothing recovered this connection will be closed by timed out eventually. To limit reading packets while Handshake, I added b008b0f from your #68 branch, basically without buffering, so now server accepts no more than initcwnd packets before Handshake complete. PTAL @ghedo. Probably we need to pick one. |
|
#68 provides more compliant solution. Closing this one. |
Update stream.send.max_data when handshake is complete.