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

net: reintroduce getter/setter for SO_LINGER to tokio::net::tcp::stream #3143

Merged
merged 5 commits into from
Nov 17, 2020

Conversation

zekisherif
Copy link
Contributor

Motivation

The ability to set_linger option on a TcpStream was removed in v0.3 so that TcpStream could be similar to std. I wasn't able to find the reason for why this set_linger was removed from std's TcpStream.

This PR reintroduces this option.

Solution

Convert the stream using it's raw fd to get a mio TcpSocket and set the option.

/// # }
/// ```
pub fn linger(&self) -> io::Result<Option<Duration>> {
let mio_socket = mio::net::TcpSocket::from(self);
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to access self.io.get_linger(), similar to how nodelay does it above.

Copy link
Contributor Author

@zekisherif zekisherif Nov 16, 2020

Choose a reason for hiding this comment

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

There are no linger options in either Mio's TcpStream which self.io refers to nor its inner which is std::net::tcp

Copy link
Member

Choose a reason for hiding this comment

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

@seanmonstar I actually don't think that's right; the methods for getting and setting SO_LINGER are defined on mio's TcpSocket type, not on its TcpStream, and self.io is a TcpStream.

That does bring up a different question, though: is there any reason it's necessary to expose these methods on Tokio's TcpStream type, rather than on its TcpSocket type? tokio::net::TcpSocket wraps mio's TcpSocket...

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I'm sorry, I misread that as TcpStream::from instead of TcpSocket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hawkw In our use case (pre tokio 0.3) we set the linger option from a stream object not TcpSocket.

I could also update this PR to also expose this on tokio::net::TcpSocket.

Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused here. set_linger is on the mio socket and PollEvented holds the mio socket? PollEvented also derefs to the mio socket, so why does self.io.get_linger() not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PolledEvented holds Mio TcpStream

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that is terribly confusing 😆

tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

I'm OK w/ this as an initial step. Ideally we could get the fn on the mio type and avoid having to do this work around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants