-
Notifications
You must be signed in to change notification settings - Fork 102
splittable RequestStream #97
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
Conversation
h3/src/quic.rs
Outdated
@@ -32,7 +32,7 @@ impl<'a, E: Error + 'a> From<E> for Box<dyn Error + 'a> { | |||
/// Trait representing a QUIC connection. | |||
pub trait Connection<B: Buf> { | |||
/// The type produced by `poll_accept_bidi()` | |||
type BidiStream: SendStream<B> + RecvStream; | |||
type BidiStream: BidiStream<B>; |
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.
If we keep the previous bounds, then implementors aren't required to provide splittable streams, just send and receive. If they implement BidiStream, as an opt-in, then the streams can be splittable.
But by not requiring it, we allow QUIC implementations that don't have an internal cheap way to split.
I redesigned the split mechanism with that use case in mind, please have another look |
stream: S, | ||
bufs: BufList<Bytes>, | ||
decoder: FrameDecoder, | ||
remaining_data: usize, | ||
/// Set to true when `stream` reaches the end. | ||
is_eos: bool, | ||
_phantom_buffer: PhantomData<B>, |
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.
Was this marker required? How come? I imagine probably yes, but I'm struggling to catch exactly where/why...
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.
impl<S, B> FrameStream<S, B>
where
S: BidiStream<B>,
B: Buf,
{
If FrameStream doesn't have the B parameter, this gives an error that B is unconstrained...
But we need the B parameter because of BidiStream.
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.
#94 would remove the need for PhantomData markers everywhere
#83