Skip to content
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

udp: add into_parts to RecvDgram #710

Merged
merged 7 commits into from
Nov 15, 2018
Merged

udp: add into_parts to RecvDgram #710

merged 7 commits into from
Nov 15, 2018

Conversation

twittner
Copy link
Contributor

@twittner twittner commented Oct 18, 2018

Add methods RecvDgram::into_inner and RecvDgram::into_parts to allow deconstructing a RecvDgram future in case it can not be driven to completion.

Motivation

If the RecvDgram future can not be driven to completion, it may become necessary to get back the UdpSocket and buffer, which is currently not possible. Since UDP is unreliable, it is advisable to use timeouts when receiving datagrams. After a timeout, one would probably retry sending and receiving, which requires the socket, however since it cannot be pulled out of RecvDgram again, RecvDgram is virtually impossible to use together with timeouts.

Solution

With into_inner and into_parts the socket and buffer can be retrieved by consuming the future. This allows to continue using those values.

If `RecvDgram` can not be driven to completion it may become necessary
to get back the `UdpSocket` it contains which is currently not possible.

This commit adds `into_inner` to do just that and `into_parts` to get
the socket as well as the buffer back. Both methods consume `RecvDgram`.

Note that after the future has completed, neither `into_inner` nor
`into_parts` must be used, or else a panic will happen.
for `RecvDgram::into_inner` and `RecvDgram::into_parts`
///
/// If called after the future has completed.
pub fn into_inner(mut self) -> UdpSocket {
let state = self.state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding common private function to get the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it would be an improvement. It would essentially look like this:

fn into_state(mut self, msg: &str) -> RcvDgramInner {
    self.state.take().expect(msg)
}

and the calls would be self.into_state("into_inner called after completion") and self.into_state("into_parts called after completion") which does not really seem clearer to me. Would you really like me to make this change?

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we want the into_inner variant? Seems like we should just have into_parts and people can drop the buffer if they don't want it.

@twittner
Copy link
Contributor Author

twittner commented Oct 26, 2018

@tobz: It is true that into_inner is redundant and only into_parts is necessary. The reason why I have included into_inner is because of similar APIs, for example in tokio_codec::Framed. That being said I do not mind removing it.

As suggested during review, `into_inner` is considered redundant and
should be removed.
@twittner twittner changed the title udp: add into_inner and into_parts to RecvDgram udp: add into_parts to RecvDgram Oct 27, 2018
@twittner
Copy link
Contributor Author

FYI: CI is failing with an internal compiler error (rustc 1.31.0-nightly) when compiling hyper which is unrelated to this PR.

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

(Yeah, that CI failure is weird.)

@tobz
Copy link
Member

tobz commented Oct 29, 2018

Although now I'm not sure I can merge this if it doesn't pass CI...

@carllerche
Copy link
Member

The strategy of returning a tuple isn't very forwards compatbile. If RecvDgram adds more fields, then it will not be possible to return those fields as part of into_parts. Using a Parts struct (e.g. here) is a way to work around this.

That said, RecvDgram will most likely not change.

Do you think it is worth returning a Parts struct or stick with a tuple?

To help future extensibility, avoid returning a tuple from
`RecvDgram::into_parts` and instead return a struct whose components can
be extracted by field name.
@twittner
Copy link
Contributor Author

Do you think it is worth returning a Parts struct or stick with a tuple?

I think it is.

@twittner
Copy link
Contributor Author

CI failure is caused by rust-lang/rust#55376.

@twittner
Copy link
Contributor Author

twittner commented Nov 6, 2018

@kpp, @tobz, @carllerche: Is there anything else I should address?

@tobz
Copy link
Member

tobz commented Nov 7, 2018

@twittner Nope, I think this is good. Unfortunately, with failing checks, only @carllerche has the power to merge it. I'll try and ping him to get it merged. 👍

@twittner
Copy link
Contributor Author

@tobz: CI was successful. Maybe you can merge this now?

@tobz tobz merged commit 09f2ac8 into tokio-rs:master Nov 15, 2018
@tobz
Copy link
Member

tobz commented Nov 15, 2018

All set! 👍

@twittner twittner deleted the recvdgram-parts branch November 16, 2018 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants