Added networking example to listen on random socket using tcp/ip #137

AndyGauge · 2017-05-19T17:34:49Z

Fixes #123

…listen-tcp-random-port

dtolnay · 2017-05-19T18:17:36Z

src/net.md

+    let listen = TcpListener::bind(socket)?;
+    let port = listen.local_addr()?;
+    println!("Listening on {}, access this port to end the program", port);
+    match listen.accept() {


Please use listen.accept()? to handle this error.

Is it idiomatic to keep the match? I've refactored it to this:

match listen.accept()? { (mut tcp_stream, addr) => { // read from the socket until connection closed by client. let mut input = String::new(); let _ = tcp_stream.read_to_string(&mut input)?; //discard byte count println!("Connection received! \r\n{:?} says {}", addr, input); Ok(()) } }

Would this work?

let (mut tcp_stream, addr) = listen.accept()?;

dtolnay · 2017-05-19T18:18:05Z

src/net.md

+        Ok((mut tcp_stream, addr)) => {
+            // read from the socket until connection closed by client.
+            let mut input = String::new();
+            let _ = tcp_stream.read_to_string(&mut input); //discard byte count


This can return an error, please handle it with ?.

dtolnay · 2017-05-19T18:19:23Z

src/net.md

+`TcpListener::accept` returns a tuple of `TcpStream` and client information
+within a `Result`.  The `Read` implmentation is used on that `TcpStream` to
+collect the request payload.  Closing the program is as easy as browsing to the
+ip:port or using telnet to send some data, pressing ctrl-] and typing quit.


Please give the telnet command to run here so that users can tests this.

dtolnay · 2017-05-19T18:20:06Z

src/net.md

+    }
+}
+
+fn run() ->  Result<()>{


There should be one space before Result<()> and one space after.

dtolnay · 2017-05-19T18:20:19Z

src/net.md

+#[macro_use]
+extern crate error_chain;
+
+use std::net::{SocketAddrV4, Ipv4Addr,TcpListener};


Please put one space after both commas.

dtolnay · 2017-05-19T18:20:44Z

src/net.md

+}
+
+fn run() ->  Result<()>{
+    let loopback = Ipv4Addr::new(127,0,0,1);


Rust style is to put spaces after commas.

dtolnay · 2017-05-19T18:22:03Z

Hey @budziq could you take over the review for this one? LMK when it's ready to merge. Thanks so much!

budziq · 2017-05-19T19:01:27Z

Will do. I'll be near a pc later today

budziq

@AndyGauge The example is very nice! I would suggest only minor changes mostly to make the description more concise.

budziq · 2017-05-19T20:11:48Z

src/net.md

+    let loopback = Ipv4Addr::new(127, 0, 0, 1);
+    // Assigning port 0 requests the OS to assign a free port
+    let socket = SocketAddrV4::new(loopback, 0);
+    let listen = TcpListener::bind(socket)?;


I would suggest renaming listen to listener

budziq · 2017-05-19T20:13:42Z

src/net.md

+    let port = listen.local_addr()?;
+    println!("Listening on {}, access this port to end the program", port);
+    let (mut tcp_stream, addr) = listen.accept()?;  //block  until requested
+    let mut input = String::new();


the connection is already accepted here. I would suggest printing
println!("Connection received on {:?}? says {}", addr);
here as it shows exactly where synchronous waiting occurs.

budziq · 2017-05-19T20:14:28Z

src/net.md

+    let mut input = String::new();
+    // read from the socket until connection closed by client.
+    let _ = tcp_stream.read_to_string(&mut input)?; //discard byte count
+    println!("Connection received! \r\n{:?} says {}", addr, input);


I would drop the \r\n and just use
println!("{:?} says {}", addr, input);
here

budziq · 2017-05-19T20:15:26Z

src/net.md

+    let (mut tcp_stream, addr) = listen.accept()?;  //block  until requested
+    let mut input = String::new();
+    // read from the socket until connection closed by client.
+    let _ = tcp_stream.read_to_string(&mut input)?; //discard byte count


I would rephrase the comment above to include the
//discard byte count

budziq · 2017-05-19T20:15:52Z

src/net.md

+```
+
+The `std` library is leveraged to make a well formed IP/port with the
+`SocketAddrV4` and `Ipv4Addr` structs.  The loopback address is a special


Please add hyperlinks for SocketAddrV4 and Ipv4Addr

budziq · 2017-05-19T20:16:25Z

src/net.md

+[`TcpListener::local_addr`].
+
+The rest of the recipe prints out the request made on the socket.
+`TcpListener::accept` returns a tuple of `TcpStream` and client information


Please add hyperlinks for TcpListener::accept and TcpStream

also what "client information"? I would suggest just writing:
(TcpStream, SocketAddr) with appropriate hyperlinks

budziq · 2017-05-19T20:17:07Z

src/net.md

+within a `Result`.  The `Read` implmentation is used on that `TcpStream` to
+collect the request payload.  Closing the program is as easy as browsing to the
+ip:port or using `telnet ip port` to send some data.  For example, if the
+program shows Listening on 127.0.0.1:11500, run `telnet 127.0.0.1 11500`.  After


I would suggest giving the telnet 127.0.0.1 11500 a separate line

budziq · 2017-05-19T20:20:01Z

src/net.md

+
+The rest of the recipe prints out the request made on the socket.
+`TcpListener::accept` returns a tuple of `TcpStream` and client information
+within a `Result`.  The `Read` implmentation is used on that `TcpStream` to


I would suggest dropping the information about Result once hyperlinks are added to other structs and functions as it makes the description overly wordy

budziq · 2017-05-19T20:25:36Z

src/net.md

+[`TcpListener::local_addr`].
+
+The rest of the recipe prints out the request made on the socket.
+`TcpListener::accept` returns a tuple of `TcpStream` and client information


I would suggest using passive voice and not describing what function returns what as reader will be able to just follow your hyperlinks. But rather write what is happening in a concise 2-4 sentence description.

budziq · 2017-05-19T20:29:55Z

src/net.md

+The rest of the recipe prints out the request made on the socket.
+`TcpListener::accept` returns a tuple of `TcpStream` and client information
+within a `Result`.  The `Read` implmentation is used on that `TcpStream` to
+collect the request payload.  Closing the program is as easy as browsing to the


I would suggest rewording the description to. "Reading on the socket with read_to_string will wait untill connection is closed" which can be tested with telnet ip port command.

budziq

Almost there. Sorry for waiting with the rest of the review but I did not want to overwhelm you. This is almost perfect already and fixing these will make it merge ready so do not give up!

budziq · 2017-05-19T21:36:12Z

src/net.md

+
+[![std-badge]][std] [![cat-net-badge]][cat-net]
+
+Give the OS the responsibility to pick an unused [registered port].  These ports


I know that you have put a lot of effort into this section but I would rather not educate readers on CS/Networking basics. We can safely assume that reader understands these concepts and came here only for the rust snippet and its very brief textual description (that would ideally be 2-4 sentences). I would suggest dropping this paragraph completely.

budziq · 2017-05-19T21:40:56Z

src/net.md

+
+The `std` library is leveraged to make a well formed IP/port with the
+[`SocketAddrV4`] and [`Ipv4Addr`] structs.  The loopback address is a special
+address that runs only on the local machine, and is available on all machines.


I would rather not try to educate the readers on what the loopback interface is. I would just stick to stating that code is listening on the loopback interface (possibly with hyperlink to some authoritative resource)

budziq · 2017-05-19T21:43:01Z

src/net.md

+In this example, the port is displayed on the console, and will listen until a
+request is made.  If you use your browser to test this program, simply enter
+the address:port into your location bar and make the request.  Because the
+program returns nothing, the browser's stop feature can be used to speed things


This paragraph is overly word and redundant with later telnet testing one.
I would remove all of it except the first sentence.

budziq · 2017-05-19T21:44:29Z

src/net.md

+port.  The address and port that the OS assigns is available using
+[`TcpListener::local_addr`].
+
+The rest of the recipe prints out the request made on the socket.


I would remove this sentence and modify the following ones to reflect what is going one step by step.

budziq · 2017-05-19T21:48:43Z

src/net.md

+[`SocketAddrV4`] and [`Ipv4Addr`] structs.  The loopback address is a special
+address that runs only on the local machine, and is available on all machines.
+
+By passing 0 to the [`TcpListener::bind`], the OS will assign an unused random


I would suggest rewording it in a more concise manner e.g:

An unused random port is requested by passing 0 to the [TcpListener::bind]. The assigned address is available via [TcpListener::local_addr].

budziq · 2017-05-19T21:51:26Z

src/net.md

+[`TcpListener::local_addr`].
+
+The rest of the recipe prints out the request made on the socket.
+[`TcpListener::accept`] returns a `(`[`TcpStream`],  [`SocketAddrV4`]`)`


The key fact about accept is that it synchronously waits for an incoming connection and returns the pair

'an incoming connection is waited synchronously by TcpListener::accept` <- another case where passive isn't the best.

budziq · 2017-05-19T21:52:49Z

src/net.md

+
+The rest of the recipe prints out the request made on the socket.
+[`TcpListener::accept`] returns a `(`[`TcpStream`],  [`SocketAddrV4`]`)`
+representing the data from the client, its IP address and port.  Reading on the


data from the client, its IP address and port.
Could be misunderstood. I would suggest something like:
"representing client connection IP address and port."

OK, so I'm going to take out the details about what the tuple is and call it the request. I think the links do a good job of explaining the details.

Does this work:

[TcpListener::accept] synchronously waits for an incoming connection and
returns a ([TcpStream], [SocketAddrV4]) representing the request.
[read_to_string] will wait until the connection is closed, flushing the socket,
which can be tested with telnet ip port. For example, if the program
shows Listening on 127.0.0.1:11500, run

Looks excellent 👍

budziq · 2017-05-19T21:54:47Z

src/net.md

+address that runs only on the local machine, and is available on all machines.
+
+By passing 0 to the [`TcpListener::bind`], the OS will assign an unused random
+port.  The address and port that the OS assigns is available using


Minor nit. Please do not mix tenses. Use present/past/future throughout the whole text.

budziq · 2017-05-19T22:00:31Z

Ok drop the passive voice comments ;) https://github.com/brson/rust-cookbook/issues/139

AndyGauge · 2017-05-19T22:25:08Z

Do you want me to rebase?

budziq · 2017-05-19T22:34:08Z

Nice work! @AndyGauge

LGTM @dtolnay

AndyGauge · 2017-05-19T22:34:29Z

Hold on, registered port reference exists.

AndyGauge · 2017-05-19T22:37:24Z

whew. Thanks @budziq

budziq · 2017-05-19T22:41:31Z

No problem. Keep them coming @AndyGauge !

dtolnay · 2017-05-19T23:18:22Z

Great work.

AndyGauge added 2 commits May 19, 2017 10:30

Added networking example to listen on random socket using tcp/ip

18e35d0

Merge branch 'master' of https://github.com/brson/rust-cookbook into …

0436206

…listen-tcp-random-port

dtolnay reviewed May 19, 2017

View reviewed changes

fixed spaces, added telnet example, and properly returned errors

e5c7db1

budziq requested changes May 19, 2017

View reviewed changes

Reword description and comments, variable names to match tense.

25b53f7

budziq requested changes May 19, 2017

View reviewed changes

Make language more concise / readable

eaf2abc

Removed unused reference

10a14ea

dtolnay merged commit 9ca2e50 into rust-lang-nursery:master May 19, 2017


		[![std-badge]][std] [![cat-net-badge]][cat-net]

		Give the OS the responsibility to pick an unused [registered port]. These ports

Added networking example to listen on random socket using tcp/ip #137

Added networking example to listen on random socket using tcp/ip #137

Conversation

AndyGauge commented May 19, 2017

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

dtolnay commented May 19, 2017

budziq commented May 19, 2017

budziq 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budziq 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

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

budziq May 19, 2017 • edited Loading

Choose a reason for hiding this comment

budziq commented May 19, 2017

AndyGauge commented May 19, 2017

budziq commented May 19, 2017

AndyGauge commented May 19, 2017

AndyGauge commented May 19, 2017

budziq commented May 19, 2017

dtolnay commented May 19, 2017

budziq May 19, 2017 •

edited

Loading