Expand docs/examples for TCP `set_nonblocking` methods.#45227

frewsxcv · 2017-10-12T02:37:24Z

Part of #44050.

rust-highfive · 2017-10-12T02:37:27Z

(rust_highfive has picked a reviewer for you, use r? to override)

sfackler · 2017-10-12T06:04:57Z

src/libstd/net/tcp.rs

Saying that the operation is pending might be a bit misleading - WouldBlock means that the request couldn't be satisfied at that moment and you need to retry later. It's not the case that the operation's been queued up and will eventually finish.

thanks for mentioning this, was a little confused how it worked behind the scenes. i addressed it in 6c20fe5 but let me know if you know a better way of wording it

kennytm · 2017-10-12T06:48:34Z

src/libstd/net/tcp.rs

tbu- · 2017-10-12T09:37:12Z

src/libstd/net/tcp.rs

It is somewhat import to specify which fcntl or ioctlsocket is used, both fcntl and ioctlsocket are some kind of "catch-all" syscall that can do wildly differing things. The fcntl FIONBIO is used on Unix platforms and the ioctlsocket FIONBIO is used on Windows.

addressed in c978e73

frewsxcv · 2017-10-14T02:33:32Z

all comments have been addressed, let me know if y'all see anything else

EDIT: i'll squash before review approval

tbu- · 2017-10-14T02:36:30Z

LGTM

sfackler · 2017-10-16T03:47:52Z

src/libstd/net/tcp.rs

I feel kind of conflicted about these examples. We don't want to get too far down the epoll/IOCP rabbit hole, but it seems pretty bad for the example here to be a thing you should never do. We could maybe have some fake wait_for_more_data() function and add something like "typically implemented via platform-specific APIs such as epoll or IOCP" into the docs?

is this sorta what you mean?

diff --git a/src/libstd/net/tcp.rs b/src/libstd/net/tcp.rs index 51269d57e6..946584bcf2 100644 --- a/src/libstd/net/tcp.rs +++ b/src/libstd/net/tcp.rs @@ -529,7 +529,10 @@ impl TcpStream { /// let num_bytes_read = loop { /// match stream.read_to_end(&mut buf) { /// Ok(b) => break b, - /// Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => (), + /// Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => { + /// wait_for_fd(); // typically implemented via platform-specific + /// // APIs such as epoll or IOCP + /// } /// Err(e) => panic!("encountered IO error: {}", e), /// }; /// }; @@ -839,13 +842,20 @@ impl TcpListener { /// for stream in listener.incoming() { /// let mut stream = match stream { /// Ok(s) => s, - /// Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => continue, + /// Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => { + /// wait_for_more_data(); // typically implemented via platform-specific + /// // APIs such as epoll or IOCP + /// continue; + /// } /// Err(e) => panic!("encountered IO error: {}", e), /// }; /// loop { /// match stream.write_all(b"foo") { /// Ok(()) => break, - /// Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => (), + /// Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => { + /// wait_for_fd(); // typically implemented via platform-specific + /// // APIs such as epoll or IOCP + /// } /// Err(e) => panic!("encountered IO error: {}", e), /// }; /// }

Yeah, something like that seems better than nothing.

frewsxcv · 2017-10-20T01:23:10Z

@sfackler latest commit should hopefully address your concerns.

i also removed the note about busy-looping now that i've added in wait_for_fd();, since afaik, this is no longer busy-looping

sfackler · 2017-10-20T01:49:10Z

src/libstd/net/tcp.rs

write_all isn't going to behave properly in a nonblocking world. If fo was successfully written but we hit a WouldBlock finishing the o, everything will be sent again the next round.

We'd also need to set the stream to nonblocking mode for the WouldBlock case to come up in the first place.

sfackler · 2017-10-20T01:49:25Z

src/libstd/net/tcp.rs

Would it be simpler to just open a socket and write to it rather than also including a listener?

this doc comment is forTcpListener::set_nonblocking, so it made sense for me to include the listener. can you expand on what you mean?

unless you're saying i shouldn't write to the stream. i could just read from it and print out the result. i'd also avoid the write_all thing from your other comment

Oh derp, sorry. I was thinking this was on write for some reason...

sfackler · 2017-10-20T23:58:51Z

src/libstd/net/tcp.rs

One last thing (sorry!) - num_bytes_read will only contain the number of bytes read on the final call, so we probably shouldn't save it here.

no reason to be sorry! i feel bad i'm dragging this on for you 😬

sfackler · 2017-10-21T00:20:05Z

src/libstd/net/tcp.rs

accept here since it's a listener.

sfackler · 2017-10-21T00:20:36Z

src/libstd/net/tcp.rs

This got added back?

before i was doing tcp writing, this is now doing tcp reading to avoid deadling with the write_all issue. i can remove reading/writing altogether and just grab the stream from the listener, which might make things simpler but not as realistic, but i'm not opposed

One option is something like Ok(s) => handle_connection(s).

sfackler · 2017-10-21T03:09:39Z

Looks good! Can you squash down to a single commit? r=me otherwise.

kennytm · 2017-10-21T05:39:54Z

src/libstd/net/tcp.rs

The , should be a ;.

[01:04:21] ---- net/tcp.rs - net::tcp::TcpListener::set_nonblocking (line 826) stdout ---- [01:04:21] error: expected one of `.`, `;`, `?`, `}`, or an operator, found `,` [01:04:21] --> net/tcp.rs:16:33 [01:04:21] | [01:04:21] 16 | handle_connection(s), [01:04:21] | ^ expected one of `.`, `;`, `?`, `}`, or an operator here [01:04:21] [01:04:21] thread 'rustc' panicked at 'couldn't compile the test', /checkout/src/librustdoc/test.rs:287:12 [01:04:21] note: Run with `RUST_BACKTRACE=1` for a backtrace. [01:04:21] [01:04:21] [01:04:21] failures: [01:04:21] net/tcp.rs - net::tcp::TcpListener::set_nonblocking (line 826)

Part of rust-lang#44050.

frewsxcv · 2017-10-21T12:58:49Z

thanks for your patience and insight @sfackler ❤️

@bors r=sfackler rollup

bors · 2017-10-21T12:58:50Z

📌 Commit fe3ed20 has been approved by sfackler

…r=sfackler Expand docs/examples for TCP `set_nonblocking` methods. Part of rust-lang#44050.

Rollup of 6 pull requests - Successful merges: #45227, #45356, #45407, #45411, #45418, #45419 - Failed merges: #45421

rust-highfive assigned aturon Oct 12, 2017

frewsxcv force-pushed the frewsxcv-set-nonblocking branch from 9c233ee to 8f0b531 Compare October 12, 2017 02:39

frewsxcv mentioned this pull request Oct 12, 2017

Improve documentation for TcpStream::set_nonblocking #44050

Closed

sfackler reviewed Oct 12, 2017

View reviewed changes

kennytm reviewed Oct 12, 2017

View reviewed changes

tbu- reviewed Oct 12, 2017

View reviewed changes

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 13, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 14, 2017

frewsxcv assigned QuietMisdreavus and unassigned aturon Oct 14, 2017

sfackler reviewed Oct 16, 2017

View reviewed changes

frewsxcv force-pushed the frewsxcv-set-nonblocking branch from 924dedb to 1ba2f1a Compare October 20, 2017 01:22

sfackler reviewed Oct 20, 2017

View reviewed changes

frewsxcv force-pushed the frewsxcv-set-nonblocking branch from 0501e46 to 97e8d00 Compare October 21, 2017 00:14

sfackler reviewed Oct 21, 2017

View reviewed changes

src/libstd/net/tcp.rs Outdated

Copy link

Member

sfackler Oct 21, 2017

Choose a reason for hiding this comment

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

accept here since it's a listener.

sfackler reviewed Oct 21, 2017

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2017

kennytm reviewed Oct 21, 2017

View reviewed changes

Expand docs/examples for TCP set_nonblocking methods.

fe3ed20

Part of rust-lang#44050.

frewsxcv force-pushed the frewsxcv-set-nonblocking branch from 439391d to fe3ed20 Compare October 21, 2017 10:13

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2017

kennytm removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 21, 2017

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2017

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Oct 21, 2017

Rollup merge of rust-lang#45227 - frewsxcv:frewsxcv-set-nonblocking, …

cf29743

…r=sfackler Expand docs/examples for TCP `set_nonblocking` methods. Part of rust-lang#44050.

frewsxcv mentioned this pull request Oct 21, 2017

Rollup of 6 pull requests #45430

Merged

bors added a commit that referenced this pull request Oct 21, 2017

Auto merge of #45430 - frewsxcv:rollup, r=frewsxcv

7e70546

Rollup of 6 pull requests - Successful merges: #45227, #45356, #45407, #45411, #45418, #45419 - Failed merges: #45421

bors merged commit fe3ed20 into rust-lang:master Oct 21, 2017

frewsxcv deleted the frewsxcv-set-nonblocking branch October 21, 2017 16:41

frewsxcv mentioned this pull request Dec 11, 2017

Clarify which TcpStream methods won't work in non-blocking mode #46597

Closed

Uh oh!

Conversation

frewsxcv commented Oct 12, 2017

Uh oh!

rust-highfive commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frewsxcv commented Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbu- commented Oct 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frewsxcv Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frewsxcv commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfackler commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frewsxcv commented Oct 21, 2017

Uh oh!

bors commented Oct 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

frewsxcv commented Oct 14, 2017 •

edited

Loading

frewsxcv Oct 19, 2017 •

edited

Loading