Add peek APIs to std::net #38983

APTy · 2017-01-11T03:26:38Z

peek

Adds "peek" APIs to std::net sockets, including:

UdpSocket.peek()
UdpSocket.peek_from()
TcpStream.peek()

These methods enable socket reads without side-effects. That is, repeated calls to peek() return identical data. This is accomplished by providing the POSIX flag MSG_PEEK to the underlying socket read operations.

refactor

This also moves the current implementation of recv_from out of the platform-independent sys_common and into respective sys/windows and sys/unix implementations. This allows for more platform-dependent implementations where necessary.

Fixes #38980

rust-highfive · 2017-01-11T03:26:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

aturon · 2017-01-11T18:31:01Z

cc @rust-lang/libs, new proposed unstable APIs for UDP.

alexcrichton

For consistency should we also be sure to add this to the TCP streams as well?

alexcrichton · 2017-01-11T23:41:03Z

src/libstd/sys/windows/net.rs

@@ -24,6 +24,8 @@ use sys_common::io::read_to_end_uninitialized;
 use sys_common::net;
 use time::Duration;

+const MSG_PEEK: c_int = 0x2;


Can this be added to the the c module with other windows bindings?

alexcrichton · 2017-01-11T23:41:40Z

src/libstd/sys_common/net.rs

@@ -53,6 +53,8 @@ use libc::MSG_NOSIGNAL;
              target_os = "haiku", target_os = "bitrig")))]
 const MSG_NOSIGNAL: c_int = 0x0;

+const MSG_PEEK: c_int = 0x2;


This seems duplicated with the Windows definition? If this is the unix definition can it be pulled from libc?

Yes, let's use libc if possible. I've added the flag only very recently in rust-lang/libc#492. How is best to make this transition? Should we hold off on this PR until the next libc release?

Oh you can just update the submodule in the repository

alexcrichton · 2017-01-11T23:42:56Z

src/libstd/sys/windows/net.rs

                -1 if c::WSAGetLastError() == c::WSAESHUTDOWN => Ok(0),
                -1 => Err(last_error()),
                n => Ok(n as usize)
            }
        }
    }

+    pub fn read(&self, buf: &mut [u8]) -> io::Result<usize> {
+        recv_with_flags(buf, 0)


This may need to be self.recv_with_flags

alexcrichton · 2017-01-11T23:43:30Z

src/libstd/sys/unix/ext/net.rs

@@ -39,6 +39,8 @@ use libc::MSG_NOSIGNAL;
              target_os = "haiku", target_os = "bitrig")))]
 const MSG_NOSIGNAL: libc::c_int = 0x0;

+const MSG_PEEK: libc::c_int = 0x2;


Can this be pulled from the libc crate?

alexcrichton · 2017-01-11T23:44:26Z

src/libstd/net/udp.rs

+    /// Successive calls return the same data.
+    ///
+    /// On success, returns the number of bytes read and the address from
+    /// whence the data came.


Could this documentation also elaborate on how this relates to MSG_PEEK?

sfackler · 2017-01-11T23:59:27Z

Unix sockets as well.

APTy · 2017-01-13T06:06:06Z

Removed the sys/unix/ext and sys/redox partial implementations, since they seem unnecessary at the moment, but let me know if you feel otherwise.

Last note from me: do we need to change #[unstable(feature = "peek", issue = "38980")] before merge? I'm not familiar with the unstable/stable APIs process.

aturon · 2017-01-31T16:48:18Z

Looks like a submodule update to liblibc got committed by accident.

aturon

Just a few nits about docs and tests; otherwise LGTM. Sorry for the long delay in reviewing!

aturon · 2017-01-31T16:50:51Z

src/libstd/net/tcp.rs

@@ -296,6 +296,29 @@ impl TcpStream {
        self.0.write_timeout()
    }

+    /// Receives data on the socket from the remote adress to which it is


It'd be good to document the return value as well.

aturon · 2017-01-31T16:55:25Z

src/libstd/net/tcp.rs

+    ///                        .expect("couldn't bind to address");
+    /// let mut buf = [0; 10];
+    /// let len = stream.peek(&mut buf).expect("peek failed");
+    /// assert_eq!(len, 10);


I'm confused by this assertion -- I'd expect there to be no bytes for peeking?

ah yeah, this probably needs some tweaking

aturon · 2017-01-31T16:55:44Z

src/libstd/net/tcp.rs

+            for _ in 1..3 {
+                assert_eq!(c.peek(&mut b).unwrap(), 1);
+            }
+            assert_eq!(c.read(&mut b).unwrap(), 1);


It'd be good to test a peek after this call to read.

aturon · 2017-01-31T16:56:46Z

src/libstd/net/udp.rs

+    /// Successive calls return the same data. This is accomplished by passing
+    /// `MSG_PEEK` as a flag to the underlying `recv` system call.
+    ///
+    /// The `connect` method will connect this socket to a remote address. This


This bit about connect should maybe move to a dedicated "Errors" section. I'd also suggest swapping the order of sentences here.

aturon · 2017-01-31T16:56:54Z

src/libstd/net/udp.rs

@@ -579,6 +603,34 @@ impl UdpSocket {
        self.0.recv(buf)
    }

+    /// Receives data on the socket from the remote adress to which it is


Docs should include info about the return value.

APTy · 2017-01-31T23:09:47Z

@aturon thanks! The liblibc update is to take advantage of the MSG_PEEK flag platform bindings recently added. Will update tests/docs shortly!

sfackler · 2017-02-03T01:30:18Z

Is this good to go?

aturon · 2017-02-03T16:55:41Z

@sfackler Not yet, one of the assertions in the doc comment needs fixing still.

APTy · 2017-02-03T17:08:11Z

@aturon I simply removed the assertion, I don't think it added much value to the documentation anyway

aturon · 2017-02-03T17:13:49Z

Thanks! Let's send it to bors.

@bors: r+

bors · 2017-02-03T17:13:50Z

📌 Commit a870c1c has been approved by aturon

bors · 2017-02-03T19:09:22Z

⌛ Testing commit a870c1c with merge c5d899f...

bors · 2017-02-03T19:18:24Z

💔 Test failed - status-appveyor

alexcrichton · 2017-02-04T19:30:05Z

@APTy looks like the tests may be failing on Travis? Also, could you squash down the commits as well?

APTy · 2017-02-04T19:36:37Z

👍 Looking into the failure, will squash as well

These methods enable socket reads without side-effects. That is, repeated calls to peek() return identical data. This is accomplished by providing the POSIX flag MSG_PEEK to the underlying socket read operations. This also moves the current implementation of recv_from out of the platform-independent sys_common and into respective sys/windows and sys/unix implementations. This allows for more platform-dependent implementations.

alexcrichton · 2017-02-04T22:09:11Z

@bors: r=aturon

bors · 2017-02-04T22:09:12Z

📌 Commit a40be08 has been approved by aturon

Add peek APIs to std::net Adds "peek" APIs to `std::net` sockets, including: - `UdpSocket.peek()` - `UdpSocket.peek_from()` - `TcpStream.peek()` These methods enable socket reads without side-effects. That is, repeated calls to `peek()` return identical data. This is accomplished by providing the POSIX flag `MSG_PEEK` to the underlying socket read operations. This also moves the current implementation of `recv_from` out of the platform-independent `sys_common` and into respective `sys/windows` and `sys/unix` implementations. This allows for more platform-dependent implementations where necessary. Fixes rust-lang#38980

Rollup of 19 pull requests - Successful merges: #38518, #38921, #38959, #38983, #39009, #39107, #39193, #39289, #39312, #39393, #39442, #39443, #39453, #39454, #39471, #39477, #39478, #39527, #39552 - Failed merges:

rust-highfive assigned aturon Jan 11, 2017

aturon added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 11, 2017

alexcrichton reviewed Jan 11, 2017

View reviewed changes

APTy changed the title ~~[WIP] Add peek APIs to std::net::UdpSocket~~ [WIP] Add peek APIs to std::net Jan 12, 2017

APTy changed the title ~~[WIP] Add peek APIs to std::net~~ Add peek APIs to std::net Jan 13, 2017

aturon suggested changes Jan 31, 2017

View reviewed changes

aturon approved these changes Feb 3, 2017

View reviewed changes

APTy force-pushed the udp-peek branch from 242e8f2 to 1b5a77b Compare February 4, 2017 02:12

APTy force-pushed the udp-peek branch from 1b5a77b to a40be08 Compare February 4, 2017 20:09

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 14 pull requests #39551

Closed

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 20 pull requests #39562

Closed

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 19 pull requests #39563

Merged

bors merged commit a40be08 into rust-lang:master Feb 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add peek APIs to std::net #38983

Add peek APIs to std::net #38983

APTy commented Jan 11, 2017 •

edited

Loading

rust-highfive commented Jan 11, 2017

aturon commented Jan 11, 2017

alexcrichton left a comment

alexcrichton Jan 11, 2017

APTy Jan 12, 2017

alexcrichton Jan 11, 2017

APTy Jan 12, 2017

alexcrichton Jan 12, 2017

alexcrichton Jan 11, 2017

alexcrichton Jan 11, 2017

alexcrichton Jan 11, 2017

sfackler commented Jan 11, 2017

APTy commented Jan 13, 2017 •

edited

Loading

aturon commented Jan 31, 2017

aturon left a comment

aturon Jan 31, 2017

aturon Jan 31, 2017

APTy Feb 1, 2017

aturon Jan 31, 2017

aturon Jan 31, 2017

aturon Jan 31, 2017

APTy commented Jan 31, 2017

sfackler commented Feb 3, 2017

aturon commented Feb 3, 2017

APTy commented Feb 3, 2017

aturon commented Feb 3, 2017

bors commented Feb 3, 2017

bors commented Feb 3, 2017

bors commented Feb 3, 2017

alexcrichton commented Feb 4, 2017

APTy commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

Add peek APIs to std::net #38983

Add peek APIs to std::net #38983

Conversation

APTy commented Jan 11, 2017 • edited Loading

peek

refactor

rust-highfive commented Jan 11, 2017

aturon commented Jan 11, 2017

alexcrichton 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

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

sfackler commented Jan 11, 2017

APTy commented Jan 13, 2017 • edited Loading

aturon commented Jan 31, 2017

aturon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APTy commented Jan 31, 2017

sfackler commented Feb 3, 2017

aturon commented Feb 3, 2017

APTy commented Feb 3, 2017

aturon commented Feb 3, 2017

bors commented Feb 3, 2017

bors commented Feb 3, 2017

bors commented Feb 3, 2017

alexcrichton commented Feb 4, 2017

APTy commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

APTy commented Jan 11, 2017 •

edited

Loading

APTy commented Jan 13, 2017 •

edited

Loading