-
Notifications
You must be signed in to change notification settings - Fork 4.7k
adds QUIC endpoint specific for turbine connections #32294
Conversation
ee0fb94
to
ba4d162
Compare
Codecov Report
@@ Coverage Diff @@
## master #32294 +/- ##
=========================================
- Coverage 82.1% 82.0% -0.1%
=========================================
Files 773 773
Lines 209793 209814 +21
=========================================
- Hits 172253 172242 -11
- Misses 37540 37572 +32 |
ba4d162
to
445a77f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! I found the whole thing really easy to read, so thanks for keeping it simple. Most of my points are small questions
let server_config = new_server_config(cert.clone(), key.clone())?; | ||
let client_config = new_client_config(cert, key)?; | ||
let mut endpoint = { | ||
let _guard = runtime.enter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this guard needed? Looking quickly into the code, I don't see that Endpoint
needs to be in a runtime context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the guard, this surprisingly panics when it executes <quinn::runtime::tokio::TokioRuntime as quinn::runtime::Runtime>::wrap_udp_socket
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, that really stinks. Could you add a comment explaining that? It isn't immediately clear from the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, just the one nit to add a comment about the runtime guard. After that, this can go in
Working towards separating out turbine QUIC from TPU.
done. |
Working towards separating out turbine QUIC from TPU.
Problem
Currently Turbine QUIC is tied to TPU implementation which makes it difficult to make changes without impacting TPU or optimize the code specifically for Turbine.
Summary of Changes
Working towards separating out Turbine QUIC from TPU, the commit adds a new QUIC endpoint implementation specific for turbine connections.
Compared to QUIC code for TPU, in this code:
This commit only adds the new endpoint constructs; following commits will: