net: add a into_std method for tokio's TcpStream #3189

liufuyang · 2020-11-28T11:36:43Z

Motivation

Solution

Add an into_inner method on PollEvented. Make sure to deregister the stream from Tokio.
Convert the mio tcp stream into an std one. Go through mio's IntoRawFd to do so.

Fixes: tokio-rs#3031 Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

Darksonn

This seems ok to me.

@Thomasdezeeuw is there a better way to convert the mio TcpStream than going through RawFd or RawSocket? I don't see one myself.

Thomasdezeeuw · 2020-12-01T20:40:48Z

@Darksonn not right now, but there is tokio-rs/mio#1401.

Darksonn · 2020-12-02T17:55:53Z

tokio/src/io/poll_evented.rs

+        let mut inner = self
+            .io
+            .take()
+            .ok_or_else(|| io::Error::from(io::ErrorKind::NotFound))?;


I see that other parts of the code just unwrap this. Is it the case that it is an Option only for the purpose of being possible to take it out in or before the destructor? @carllerche

Yes, a panic here is fine as it would be an internal bug.

I believe @Darksonn is correct — this shouldn't ever be None unless the PollEvented is being dropped or into_std has been called, so this can just be an unwrap()...

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

liufuyang · 2020-12-05T10:51:44Z

Hey @Darksonn, thanks for the comments above. I also added a doc test so to make a std TcpStream and let a client read it. Do you think the test code is solid enough? Or should I add a normal unit test for into_std as well? (If so, which file should I start adding this test?) Thank you :)

Darksonn

I have some stylistic comments on the doc-test.

Darksonn · 2020-12-05T13:16:21Z

tokio/src/net/tcp/stream.rs

+    /// #       let mut stream: TcpStream = TcpStream::connect("127.0.0.1:8080").await.unwrap();
+    /// #       let _ = stream.write(b"Hello world!").await;
+    /// #   });
+    ///     let mut data = [0 as u8; 50];


Suggested change

/// let mut data = [0 as u8; 50];

/// let mut data = [0u8; 50];

Darksonn · 2020-12-05T13:17:20Z

tokio/src/net/tcp/stream.rs

+    /// #[tokio::main]
+    /// async fn main() -> Result<(), Box<dyn Error>> {
+    /// #   tokio::spawn(async {
+    /// #       tokio::time::sleep(Duration::from_millis(50)).await;


I think you can avoid this sleep if you spawn it after calling bind. Also, can you await the returned JoinHandle at the end of the test to make sure the test fails if the task panics?

Darksonn · 2020-12-05T13:17:38Z

tokio/src/net/tcp/stream.rs

+    ///     let (tokio_tcp_stream, _) = listener.accept().await?;
+    ///
+    ///     let mut std_tcp_stream = tokio_tcp_stream.into_std()?;
+    /// #   tokio::time::sleep(Duration::from_millis(100)).await;


This sleep should be unnecessary.

Darksonn · 2020-12-05T13:18:02Z

tokio/src/net/tcp/stream.rs

+    /// #   tokio::spawn(async {
+    /// #       tokio::time::sleep(Duration::from_millis(50)).await;
+    /// #       let mut stream: TcpStream = TcpStream::connect("127.0.0.1:8080").await.unwrap();
+    /// #       let _ = stream.write(b"Hello world!").await;


Suggested change

/// # let _ = stream.write(b"Hello world!").await;

/// # stream.write(b"Hello world!").await.unwrap();

Seems I need this otherwise I see this during test?

running 1 test test src/net/tcp/stream.rs - net::tcp::stream::TcpStream::into_std (line 191) ... FAILED failures: ---- src/net/tcp/stream.rs - net::tcp::stream::TcpStream::into_std (line 191) stdout ---- error: unused `std::result::Result` that must be used --> src/net/tcp/stream.rs:204:7 | 15 | stream.write(b"Hello world!").await; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: the lint level is defined here --> src/net/tcp/stream.rs:190:9 | 1 | #![deny(warnings, rust_2018_idioms)] | ^^^^^^^^ = note: `#[deny(unused_must_use)]` implied by `#[deny(warnings)]` = note: this `Result` may be an `Err` variant, which should be handled error: aborting due to previous error Couldn't compile the test. failures: src/net/tcp/stream.rs - net::tcp::stream::TcpStream::into_std (line 191)

You need the unwrap() too.

Aha, thanks 😄

Darksonn · 2020-12-05T13:18:53Z

tokio/src/net/tcp/stream.rs

+    ///     let mut std_tcp_stream = tokio_tcp_stream.into_std()?;
+    /// #   tokio::time::sleep(Duration::from_millis(100)).await;
+    ///     let size = std_tcp_stream.read(&mut data)?;
+    /// #   assert_eq!("Hello world!", std::str::from_utf8(&data[0..size])?);


Suggested change

/// # assert_eq!("Hello world!", std::str::from_utf8(&data[0..size])?);

/// # assert_eq!(b"Hello world!", &data[0..size]);

Aha, nice :)

Darksonn · 2020-12-05T13:21:55Z

tokio/src/net/tcp/stream.rs

+    /// #   tokio::time::sleep(Duration::from_millis(100)).await;
+    ///     let size = std_tcp_stream.read(&mut data)?;
+    /// #   assert_eq!("Hello world!", std::str::from_utf8(&data[0..size])?);
+    /// #   Ok(())


Without this, it wont compile if the user pastes it into the playground.

Suggested change

/// # Ok(())

/// Ok(())

Darksonn · 2020-12-05T13:25:51Z

I think it would be worth adding a test that the TcpStream still works if you convert it back and forth as in tokio → std → tokio. You can put the test in tokio/tests/tcp_into_std.rs.

Besides that, I think the doc-test captures what I'm looking for.

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

Darksonn

I think this looks good, besides a missing space.

Darksonn · 2020-12-06T12:41:23Z

tokio/src/net/tcp/stream.rs

+    ///     std_tcp_stream.set_nonblocking(false)?;
+    ///     std_tcp_stream.read_exact(&mut data)?;
+    /// #   assert_eq!(b"Hello world!", &data);
+    ///    Ok(())


Suggested change

/// Ok(())

/// Ok(())

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

liufuyang force-pushed the issue-3031 branch 3 times, most recently from 2c5c087 to 86382a4 Compare November 28, 2020 11:44

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-net Module: tokio/net labels Nov 28, 2020

liufuyang force-pushed the issue-3031 branch 2 times, most recently from 6b2147e to cf99d4e Compare November 28, 2020 12:48

liufuyang changed the title ~~Add a into_std method for tokio's TcpStream~~ io: add a into_std method for tokio's TcpStream Nov 28, 2020

liufuyang force-pushed the issue-3031 branch from cf99d4e to 6763153 Compare November 28, 2020 12:54

io: add a into_std method for tokio's TcpStream

34ac3e5

Fixes: tokio-rs#3031 Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

liufuyang force-pushed the issue-3031 branch from 6763153 to 34ac3e5 Compare November 28, 2020 13:13

Darksonn approved these changes Nov 28, 2020

View reviewed changes

Darksonn reviewed Dec 2, 2020

View reviewed changes

liufuyang added 2 commits December 5, 2020 11:38

Add doc test for into_std

4c1915f

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

Just use unwrap()

7f269b6

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

Darksonn reviewed Dec 5, 2020

View reviewed changes

liufuyang added 4 commits December 6, 2020 10:51

Remove sleep code in doc test

d878618

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

Use port 34254 rather than 8080 for doc test

c60f830

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

Add test tcp_into_std.rs

cf55e07

Fix up doc

5d72d4a

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

liufuyang force-pushed the issue-3031 branch from 9395c41 to 5d72d4a Compare December 6, 2020 11:54

Minor fix

7e41fa2

Darksonn approved these changes Dec 6, 2020

View reviewed changes

Minor fix

e12e522

Signed-off-by: Fuyang Liu <liufuyang@users.noreply.github.com>

Darksonn merged commit 0707f4c into tokio-rs:master Dec 6, 2020

Darksonn changed the title ~~io: add a into_std method for tokio's TcpStream~~ net: add a into_std method for tokio's TcpStream Dec 6, 2020

liufuyang deleted the issue-3031 branch December 6, 2020 13:51

bdonlan mentioned this pull request Dec 14, 2020

chore: prepare v0.3.6 release #3276

Merged

19h mentioned this pull request Feb 14, 2021

proxy/transport: use into_std in set_keepalive_or_warn linkerd/linkerd2-proxy#916

Closed

dependabot bot mentioned this pull request Mar 18, 2021

Bump tokio from 0.2.24 to 0.3.7 in /graph-data.rs openshift/cincinnati-graph-data#711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: add a into_std method for tokio's TcpStream #3189

net: add a into_std method for tokio's TcpStream #3189

liufuyang commented Nov 28, 2020

Darksonn left a comment

Thomasdezeeuw commented Dec 1, 2020

Darksonn Dec 2, 2020

carllerche Dec 2, 2020

hawkw Dec 2, 2020

liufuyang commented Dec 5, 2020

Darksonn left a comment

Darksonn Dec 5, 2020

Darksonn Dec 5, 2020

Darksonn Dec 5, 2020

Darksonn Dec 5, 2020

liufuyang Dec 6, 2020 •

edited

Loading

Darksonn Dec 6, 2020

liufuyang Dec 6, 2020

Darksonn Dec 5, 2020

liufuyang Dec 6, 2020

Darksonn Dec 5, 2020

Darksonn commented Dec 5, 2020

Darksonn left a comment

Darksonn Dec 6, 2020

	/// let mut data = [0 as u8; 50];
	/// let mut data = [0u8; 50];

	/// # let _ = stream.write(b"Hello world!").await;
	/// # stream.write(b"Hello world!").await.unwrap();

	/// # assert_eq!("Hello world!", std::str::from_utf8(&data[0..size])?);
	/// # assert_eq!(b"Hello world!", &data[0..size]);

net: add a into_std method for tokio's TcpStream #3189

net: add a into_std method for tokio's TcpStream #3189

Conversation

liufuyang commented Nov 28, 2020

Motivation

Solution

Darksonn left a comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufuyang commented Dec 5, 2020

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufuyang Dec 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn commented Dec 5, 2020

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufuyang Dec 6, 2020 •

edited

Loading