-
Notifications
You must be signed in to change notification settings - Fork 76
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
simplify traits #200
simplify traits #200
Conversation
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.
The poll merger looks good to me. Since we need to document expectations for different close scenarios anyway, it may make sense to handle the close type situation in this PR as well. I would recommend the enum proposal from #177 B2 (replace Error with Close enum).
In any case, I think the expectation that the quic implementation always emits a Close
/Error
via poll_incoming
on termination should be documented.
Stream opening itself can't fail (only block), unless the connection is terminated. Therefore I think it would be good to remove the errors from poll_open_*
. That removes any ambiguity how Closes are communicated (only via poll_incoming
). I'm not even convinced returning an Option
is necessary. Just Poll::Pending should be fine as well.
/// | ||
/// Returning `None` implies the connection is closing or closed. | ||
fn poll_accept_bidi( | ||
/// Poll the connection for incoming streams. |
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.
Add: /// Returning an Err implies the connection is closing or closed.
Thanks for the review.
I will continue to work on this and implement #177 B2. I Think this are my next tasks:
|
@Ruben2424 Is this PR in a state where it's ready for review? |
@dongcarl you can review the changes if you want. But there is still some work left to improve the error types. |
…type. Maybe some of this stuff must be undone.
I think i will not continue on this as one big pr. Instead I probably will split them up into multiple smaller ones which are easier to review. |
This is a not 100% working change to explore some of the proposed changes from #177.