-
-
Notifications
You must be signed in to change notification settings - Fork 487
feat(datagrams): add sendable/readable futures and try_send/try_recv methods #2366
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
base: main
Are you sure you want to change the base?
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.
Per discussion in #2351, it's not clear what the try_ methods add. I don't want to maintain two copies of every single async function when it's straightforward enough to poll the futures we already have.
If you're not comfortable relying on that without a documented guarantee, feel free to propose some documentation, as @djc suggested. It would be very strange for any future that could complete immediately to return Pending instead, but I don't mind explicitly committing not to.
quinn/src/connection.rs
Outdated
| /// Attempts to receive an application datagram | ||
| /// | ||
| /// If there are no readable datagrams, this will return [TryReceiveDatagramError::WouldBlock] | ||
| pub fn try_read_datagram(&self) -> Result<Bytes, TryReceiveDatagramError> { |
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.
Result<Option<Bytes>, ConnectionError> seems much less error-prone, since it separates the exceptional case.
| Ok(()) | ||
| } | ||
|
|
||
| /// Sets the send_blocked flag to true |
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 send_blocked flag is not part of the public API, so this is not helpful documentation. Maybe try an imperative phrasing of what you wrote just below?
quinn/src/connection.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// Creates a future, resolving as soon as a readable datagram is buffered |
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.
Don't prefix async function documentation with "Creates a future"; it doesn't add information.
Related Issues:
datagram_readable&datagram_writeable&try_send/recv_datagram#2351datagram_sendable)Additions:
Connection::try_send_datagramandConnection::try_read_datagram, returningWouldBlockwhen the operation cannot complete immediatelyConnection::datagram_readable, resolving as soon as any datagram is readableConnection::datagram_sendable, resolving as soon as a specified number of bytes are writable