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
[WebTransport server] Make server-initiated bidirectional streams work #30795
[WebTransport server] Make server-initiated bidirectional streams work #30795
Conversation
@nidhijaju Could you check this works around the problem you got? Also could you make sure this doesn't break other tests? @yutakahirano PTAL after this is confirmed to be working. |
Yes, I've checked that this workaround works well to temporarily solve the problem I had found. I also tested it with all the other existing tests we had, and it didn't break any of them. :) |
@@ -187,6 +187,13 @@ def create_bidirectional_stream(self) -> int: | |||
""" | |||
stream_id = self._http.create_webtransport_stream( | |||
session_id=self.session_id, is_unidirectional=False) | |||
# TODO(bashi): Remove this workaround when aioquic supports receiving |
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.
This will be broken when the bug is fixed on aioquic. How about guarding the block with
if stream.frame_type is None:
or
if stream.frame_type is None and stream.session_id is None:
?
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.
I slightly prefer to assert because these asserts force us to remove these workarounds when we update aioquic.
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.
OK, then LGTM!
Thank you @nidhijaju for checking and @yutakahirano for review! I'm going to merge this. |
The failure seems unrelated to this change. I'm going to merge this. |
No description provided.