quic transport support #64

juzi5201314 · 2022-01-05T12:00:29Z

No description provided.

yujqiao

Generally looks good. But your commit history is misformed, please rebase on the main branch.

yujqiao · 2022-01-06T02:55:24Z

Cargo.toml

 server = []
 client = []
 tls = ["tokio-native-tls"]
 noise = ["snowstorm", "base64"]
+quic = ["quinn", "rustls", "rustls-pemfile", "futures-util"]


Can we use native-tls, which rathole already depends on? In that way, we can drop the dependencies of rustls and other things.

It looks like it should be possible, but I need time to manually write the code between native-tls and quinn.

🤔 quinn chooses to only support rustls?

🤔 quinn 选择只支持 rustls？

No, rustls is optional, but quinn does not provide bindings for other similar crates other than rustls. So using native-tls should be feasible, but it cannot be used out of the box.

emmm, unfortunately I don’t know much about this, it seems to implement the relevant interface in https://github.com/quinn-rs/quinn/blob/main/quinn-proto/src/crypto.rs You can replace rustls, but I don't have this knowledge (:

yujqiao · 2022-01-06T02:57:51Z

tests/integration_test.rs

@@ -144,6 +146,9 @@ async fn test(config_path: &'static str, t: Type) -> Result<()> {
    server_shutdown_tx.send(true)?;
    let _ = tokio::join!(server);

+    // Wait for the server connection to be closed (quic)
+    time::sleep(Duration::from_millis(2500)).await;


Do you figure out why this is needed? server has been joined. And connections should have already been closed.

Because quinn actually wraps the udp socket in the arc and puts it in the tokio task to run. So after the endpoint drop, the udp socket will not drop immediately. If you start the server again at this time, you will get the port reuse error.

Then this is an issue for the hot-reloading feature. If a full restart is trigger, the new instance will try to bind on the same address and fail, which causes rathole to abort.

Any way to shutdown the UDP socket immediately, along with the server? If quinn exposes such API, maybe we call it in the drop of QuicTransport.

I didn't find the relevant method in the source code and documentation, but I can send an issue to ask.

Some bug fixes has been merged into the main branch. I don't know if that's relevant but maybe you want to do a rebase.

ok, I am taking an exam these few days, I will continue to do this later.

I suspect there's a short interval for all tokio tasks to shutdown, but normally that's not an issue because every CI that has been run so far is successful. 2.5s is too long for that. Anyway, I will find time to investigate this in the original code.

yujqiao · 2022-01-06T02:59:48Z

src/client.rs

@@ -6,7 +6,7 @@ use crate::protocol::{
    self, read_ack, read_control_cmd, read_data_cmd, read_hello, Ack, Auth, ControlChannelCmd,
    DataChannelCmd, UdpTraffic, CURRENT_PROTO_VRESION, HASH_WIDTH_IN_BYTES,
 };
-use crate::transport::{TcpTransport, Transport};
+use crate::transport::{QuicTransport, TcpTransport, Transport};


This line will fail when quic feature is not enabled. See use below for TLS.

emillynge · 2022-01-09T10:01:28Z

Hello, I had the same idea as you a week ago :)

Having just completed my own QUIC implementation, I noticed this PR before going to open my own PR.

Should I open a "competing" PR? It may make it easier to compare our approaches.
See my branch here https://github.com/emillynge/rathole/tree/quic

emillynge · 2022-01-09T10:40:38Z

I had a thought by the way.

A very nice feature of QUIC is that it can be used in a partially-reliable way.
Using bi-streams (As I also did because that is the easiest solution) is a ordered reliable transport, meaning that:

all packets will be received (QUIC will retransmit if packet is lost)
all packets will be read in the order they are sent.

Now, this is nice (almost necessary) for the control channel. But it is actually a problem for the data channel.

Retransmission of TCP packets will make it seem as if there is 0 loss from point of view of the original sender, which will make it impossible for them to detect congestion. (see TCP-meltdown)

having all packets be ordered will introduce head-of-line blocking in the transport itself. If we are carrying UDP, this is very unfortunate. If we are carrying TCP, then we now have 2 layers of HOL blocking, where just one should suffice.

What we may be able to do is to have all the control channel communication on the bi stream already implemented, but then add an additional unreliable stream for the data channel using pure datagrams quinn::connection::Connection.send_datagram.
this method purports to "Transmit data as an unreliable, unordered application datagram
This would require a change to the Transport trait, such that accept and connect returns a tuple
(Self::Stream, Option<Stream>) where the second stream is a assumed to be an unordered unreliable stream that may be used for data channel if it is present.

@rapiz1 1) should I move this thought/comment somewhere else (Issue??)
2) would you be open to making sucha change to the Transport trait?

yujqiao · 2022-01-09T10:59:26Z

@emillynge I've already seen the congestion causing problems with TCP over TCP and wondered if there's a good solution. Your proposal sounds great.

should I move this thought/comment somewhere else (Issue??)

Yeah. I think you should do that.

would you be open to making sucha change to the Transport trait?

I'm open to such a change. But you should keep #84 in mind when redesigning the trait. I'm recently planning to make a minor change to the trait but haven't find the time yet, and it may have conflicts with QUIC. I will post my primitive thoughts in #84

yujqiao · 2022-01-12T09:19:33Z

Note that trait transport has been changed in #93

juzi5201314 · 2022-01-12T16:40:17Z

hi, @emillynge is also doing this and he seems to have some new ideas, can I close this pr and keep doing this by emillynge?
If there are two people doing this at the same time, maybe it will lead to implementation issues conflict.
cc @rapiz1

emillynge · 2022-01-12T21:49:00Z

@juzi5201314 I have now created a PR.
If you want to continue working on QUIC, then I can give you push access to my branch.
You can also contribute by reviewing the PR.

juzi5201314 · 2022-01-13T04:09:53Z

@juzi5201314我现在已经创建了一个 PR。如果你想继续在 QUIC 上工作，那么我可以给你推送访问我的分支的权限。您也可以通过查看 PR 来做出贡献。

👍

yujqiao and others added 3 commits December 28, 2021 22:10

docs: update benchmark and add about memory usage

e70983a

docs: typo

976b040

quic support

f033b56

juzi5201314 mentioned this pull request Jan 5, 2022

quic transport support #65

Open

yujqiao requested changes Jan 6, 2022

View reviewed changes

emillynge mentioned this pull request Jan 12, 2022

WIP: Change Transport to allow for implementing partially reliable transport like QUIC #95

Open

emillynge mentioned this pull request Jan 12, 2022

QUIC support #98

Open

juzi5201314 closed this Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

quic transport support #64

quic transport support #64

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quic transport support #64

quic transport support #64

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!