Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make accept of Transport cancel safe #84

Closed
rapiz1 opened this issue Jan 8, 2022 · 0 comments · Fixed by #93
Closed

Make accept of Transport cancel safe #84

rapiz1 opened this issue Jan 8, 2022 · 0 comments · Fixed by #93
Labels
bug Something isn't working design Design discussion needed

Comments

@rapiz1
Copy link
Owner

rapiz1 commented Jan 8, 2022

This is unlikely to occur. But if a hot-reload for a service and a TCP connection arrives at the same time, the TCP connection will be dropped.

rathole/src/server.rs

Lines 140 to 143 in 1240dd8

tokio::select! {
// FIXME: This should be cancel safe.
// Wait for incoming control and data channels
ret = self.transport.accept(&l) => {

A primitive thought is to turn Transport::accept into Transport::handshake and make it accept a raw TCP connection. And use TCPListener::accept in select because it is cancel safe. However it breaks some abstraction and assumes the underlying protocol is TCP, which causes trouble for UDP-based transport, like QUIC. So maybe the trait can be redesigned in another way. Thoughts will be welcome.

Or keep the trait unchanged and write a future to make Transport::accept cancel safe. I don't know if that's feasible. For TCP, it's impossible to put an accepted connection back to the system backlog. And generally handshake consists of multiple packets. I doubt it can made cancel safe without altering the trait. Redesigning the trait seems the way to go.

@rapiz1 rapiz1 added the bug Something isn't working label Jan 8, 2022
@rapiz1 rapiz1 added the design Design discussion needed label Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working design Design discussion needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant