time-wait handler should take precedence over OnFailedToDispatchPacket

[ravenblackx]

I think the order in quic_dispatcher could be improved, here

This block allows an overridden dispatcher to catch a packet that doesn't match a connection ID and perform an alternative action; I'm thinking about using this in Envoy to facilitate hot restart, wherein we would want to pass new connections to the new instance, but continue to pass packets for existing connections to their respective sessions.

With the code as it is now, this would mostly work, but in the case of a connection being in time-wait state, we would incorrectly pass the packet to the new instance.

If I'm understanding things correctly, it seems to me that a packet which can be associated with a time-wait connection is correctly dispatched by doing what time_wait_list_manager_ does with it, so any packet which could take that branch should not fall into the OnFailedToDispatchPacket handler.

Switching the order of the two blocks would achieve this.

[ravenblackx]

If that would be considered too dangerous of a change since it's possible (though unlikely) that someone is relying on the behavior being how it currently is, adding OnFailedToDispatchPacket2 to OnUnrecognizedConnectionId or something like that, before StatelesslyTerminateConnection would also address the problem.

[danzh2010]

@yangfanud

[danzh2010]

Discussed with @yangfanud offline, forwarding packet at the current call site of OnFailedToDispatchPacket() is probably okay. Skipping time wait list checking should only affect closed connections whose CONNECTION_CLOSE frame gets lost, which will be retransmitted by the time wait list upon receiving more packets from the peer. Forwarding those packets to the child process should be fine given they are short header packets and will be dropped anyway.