> = vec![];
for op_stream in TcpListener::bind("127.0.0.1:8000")?.incoming() {
let stream = match op_stream {
Ok(stream) => stream,
Err(_) => continue
};
let mut client = Client::new(stream, |x| println!("Message = {}", x));
let flag = Arc::downgrade(&client.alive_handle);
thread::spawn(move || client.run());
alive_flags.push(flag);
alive_flags.retain(|x| x.upgrade().is_some());
println!("Number of threads: {}", alive_flags.len());
}
Ok(())
}
```
For reference, [here's the Python code](https://gist.github.com/MatthewScholefield/b45c299a125b9792fd6ab7a4354ba2c6) for simulating clients connecting and disconnecting.
### Updates:
- While I was hesitant at first, it seems like implementing a new method is pretty common. So I should probably have a `Client::new`.
- Exceptions should be properly handled. I've just updated the code with this
- Of course `usize` is platform dependent so I've replaced it with the more applicable `u32`
- Pass small arrays by value instead of reference
- Redundant keys when initializing variables of the same name in a struct
---
### Author:  [@hellow](https://users.rust-lang.org/u/hellow)
#### Post date: [April 3, 2019, 6:20am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/2 "2019-04-03T06:20:40Z")
In Rust, we don't use `return 3;` as last statement, instead we just write `3`.
```rust
fn read_message_size(&mut self) -> usize {
let mut bytes = [0; 4];
self.stream.read(&mut bytes).unwrap();
bytes_to_usize(&mut bytes)
}
fn get_next_message(&mut self, size: usize) -> String {
let mut data = vec![0; size];
let _ = self.stream.read(&mut data);
str::from_utf8(&data).unwrap().to_owned()
}
```
[Why is using return as the last statement in a function considered bad style?](https://stackoverflow.com/questions/27961879/why-is-using-return-as-the-last-statement-in-a-function-considered-bad-style) (on SO).
* * *
```rust
self.stream.read(&mut bytes).unwrap();
// ...
let _ = self.stream.read(&mut data);
```
is incorrect, because `read` does not necessarily read the right amount of bytes. That's why it returns a `Result` (which you should not ignore, like you do right now!). Instead use [`read_exact`](https://doc.rust-lang.org/std/io/trait.Read.html#method.read_exact).
All in all, you should familiarise yourself with the error handling concept in Rust, espcially `Result` and `Option`. See [chapter 9](https://doc.rust-lang.org/stable/book/ch09-00-error-handling.html) of the book.
* * *
```rust
let mut bytes = [0; 4];
// ...
bytes_to_usize(&mut bytes)
```
I don't think that you understood what `usize` is. It's an platform dependent type, which size is how many bytes it takes to reference any location in memory. E.g. 8 bytes on a typical x86\_64 platform on desktops. Therefore your 4 byte array is wrong here. You either want to expand that to 8 bytes or use `u32` instead.
* * *
After looking more closely to your code, you should read [the book](https://doc.rust-lang.org/stable/book/), because it seems that you haven't actually looked in it. There are some style decisions which are very un-rusty and considered bad. Please read it 🙂
---
### Author:  [@Matthew-S](https://users.rust-lang.org/u/Matthew-S)
#### Post date: [April 3, 2019, 8:22am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/3 "2019-04-03T08:22:23Z")
Thanks for the suggestions! However, exception handling and incorrect type usages don't directly address the core of what I'm asking as they don't affect much of the design of the code. In any case, I've made the suggestions you've mentioned. I'll continue reading the book to see if I can figure out some of these un-rusty style decisions. If you wouldn't mind, could you point out one or two of the biggest style improvements that could be made?
---
### Author:  [@17cupsofcoffee](https://users.rust-lang.org/u/17cupsofcoffee)
#### Post date: [April 3, 2019, 9:58am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/4 "2019-04-03T09:58:14Z")
I'd recommend installing [clippy](https://github.com/rust-lang/rust-clippy) - it'll help you find common issues with code style/correctness.
I ran it on your code via the [playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9fd18143fdf7d9cd075636a562d8e1b3) (it's in the tools menu), and some of the suggestions made were:
- When you're initializing a struct, instead of doing `stream: stream`, you can just say `stream` as a shorthand.
- `[u8; 4]` implements `Copy`, and it's small enough that passing it by reference to `bytes_to_u32` is probably less performant than just passing it by value.
- The `as` operator will sometimes do lossy conversions, which can cause bugs if you change the type of a variable. You can use functions like `u32::from` to do lossless conversions.
- You're still not handling the returned amount of bytes `read` in `read_message_size` - Clippy actually catches this and throws up a big honking error message to make sure you know it's a no-no 🙂
- If you're not familiar with why - calling `read` sometimes will only _partially_ fill the array, meaning that there could still be stale data from the previous read left in there. You either need to handle this explicitly, or say 'keep reading until the buffer is full' by calling `read_exact`.
[Here's a version of your code with these things fixed](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=840adcb71c1342f1aee7179749ef9821) - there may be other changes that could be made (don't have time to dig too deep right now), but this should get some of the low-hanging fruit out of the way.
---
### Author:  [@Matthew-S](https://users.rust-lang.org/u/Matthew-S)
#### Post date: [April 3, 2019, 10:31am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/5 "2019-04-03T10:31:57Z")
Thanks! Just updated with all those suggestions; clippy is really nice (wonder if the name pays tribute to Microsoft Office :P). Funny enough, I was actually using that exact same playground but didn't notice those tools.
---
### Author:  [@mmlinford](https://users.rust-lang.org/u/mmlinford)
#### Post date: [April 4, 2019, 3:35am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/6 "2019-04-04T03:35:18Z")
So is the only reason for all of the `alive_flags` fun so that you can keep track of the total number of threads? To be honest it took me a little while to figure out what was going on there :-).
If all you're trying to do is keep track of the thread count, here's an implementation that uses `Arc>` (Arc + Mutex go together a lot) and the `Drop` trait:
```rust
use std::error::Error;
use std::io;
use std::io::Read;
use std::net::{TcpListener, TcpStream};
use std::str;
use std::sync::{Arc, Mutex};
use std::thread;
type BoxResult = Result>;
/// Interpret 4 bytes in little endian as a u32 number
fn bytes_to_u32(array: [u8; 4]) -> u32 {
u32::from(array[0])
+ (u32::from(array[1]) << 8)
+ (u32::from(array[2]) << 16)
+ (u32::from(array[3]) << 24)
}
/// TCP socket to a client using our protocol
struct Client {
stream: TcpStream,
on_message: fn(String),
total_count: Arc>,
}
impl Client {
fn new(stream: TcpStream, on_message: fn(String), total_count: Arc>) -> Client {
Client {
stream,
on_message,
total_count,
}
}
fn run(&mut self) {
{
let mut guard = self.total_count.lock().unwrap();
*guard += 1;
}
loop {
let op_size = self.read_message_size();
if op_size.is_err() {
break;
}
let size = op_size.unwrap();
let next_msg = self.get_next_message(size);
match next_msg {
Ok(next_msg) => (self.on_message)(next_msg),
Err(e) => println!("Error getting message: {}", e),
}
}
}
fn read_message_size(&mut self) -> BoxResult {
let mut bytes = [0; 4];
self.stream.read_exact(&mut bytes)?;
Ok(bytes_to_u32(bytes) as usize)
}
fn get_next_message(&mut self, size: usize) -> BoxResult {
let mut data = vec![0; size];
self.stream.read_exact(&mut data)?;
Ok(str::from_utf8(&data)?.to_owned())
}
}
impl Drop for Client {
fn drop(&mut self) {
let mut guard = self.total_count.lock().unwrap();
*guard -= 1;
}
}
fn main() -> io::Result<()> {
let thread_count = Arc::new(Mutex::new(0));
for op_stream in TcpListener::bind("127.0.0.1:8000")?.incoming() {
let stream = match op_stream {
Ok(stream) => stream,
Err(_) => continue,
};
let mut client = Client::new(
stream,
|x| println!("Message = {}", x),
thread_count.clone(),
);
thread::spawn(move || client.run());
{
let guard = thread_count.lock().unwrap();
println!("Number of threads: {}", *guard);
}
}
Ok(())
}
```
---
### Author:  [@mmlinford](https://users.rust-lang.org/u/mmlinford)
#### Post date: [April 4, 2019, 3:41am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/7 "2019-04-04T03:41:17Z")
Although note that you might no want to call `.unwrap()` like I did 🙂
---
### Author:  [@hellow](https://users.rust-lang.org/u/hellow)
#### Post date: [April 4, 2019, 6:40am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/8 "2019-04-04T06:40:29Z")
> [@mmlinford](#):
>
> `Arc>`
Why not [`AtomicUsize`](https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html)
> [@mmlinford](#):
>
> ```rust
> Ok(bytes_to_u32(bytes) as usize)
>
> ```
There is [`u32::from_le_bytes`](https://doc.rust-lang.org/std/primitive.u32.html#method.from_le_bytes)
---
### Author:  [@mmlinford](https://users.rust-lang.org/u/mmlinford)
#### Post date: [April 4, 2019, 1:44pm UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/9 "2019-04-04T13:44:19Z")
> [@hellow](#):
>
> Why not [`AtomicUsize`](https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html)
Because I've almost only ever seen/used `Arc>` with structs and completely forgot that `AtomicUsize` existed 🙂 Thanks for the tip!
> [@hellow](#):
>
> There is [`u32::from_le_bytes`](https://doc.rust-lang.org/std/primitive.u32.html#method.from_le_bytes)
Yup, that part was just copy-pasted from the original post. I just added my `Arc>` stuff.
---
### Author:  [@17cupsofcoffee](https://users.rust-lang.org/u/17cupsofcoffee)
#### Post date: [April 4, 2019, 4:14pm UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/10 "2019-04-04T16:14:03Z")
In case anyone else was curious, here's a version with @hellow's suggested changes:
```rust
use std::error::Error;
use std::io;
use std::io::Read;
use std::net::{TcpListener, TcpStream};
use std::str;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::Arc;
use std::thread;
type BoxResult = Result>;
/// TCP socket to a client using our protocol
struct Client {
stream: TcpStream,
on_message: fn(String),
total_count: Arc,
}
impl Client {
fn new(stream: TcpStream, on_message: fn(String), total_count: Arc) -> Client {
Client {
stream,
on_message,
total_count,
}
}
fn run(&mut self) {
self.total_count.fetch_add(1, Ordering::SeqCst);
loop {
let op_size = self.read_message_size();
if op_size.is_err() {
break;
}
let size = op_size.unwrap();
let next_msg = self.get_next_message(size);
match next_msg {
Ok(next_msg) => (self.on_message)(next_msg),
Err(e) => println!("Error getting message: {}", e),
}
}
}
fn read_message_size(&mut self) -> BoxResult {
let mut bytes = [0; 4];
self.stream.read_exact(&mut bytes)?;
Ok(u32::from_le_bytes(bytes) as usize)
}
fn get_next_message(&mut self, size: usize) -> BoxResult {
let mut data = vec![0; size];
self.stream.read_exact(&mut data)?;
Ok(str::from_utf8(&data)?.to_owned())
}
}
impl Drop for Client {
fn drop(&mut self) {
self.total_count.fetch_sub(1, Ordering::SeqCst);
}
}
fn main() -> io::Result<()> {
let thread_count = Arc::new(AtomicUsize::new(0));
for op_stream in TcpListener::bind("127.0.0.1:8000")?.incoming() {
let stream = match op_stream {
Ok(stream) => stream,
Err(_) => continue,
};
let mut client = Client::new(
stream,
|x| println!("Message = {}", x),
Arc::clone(&thread_count),
);
thread::spawn(move || client.run());
println!("Number of threads: {}", thread_count.load(Ordering::SeqCst));
}
Ok(())
}
```
You might be able to relax some of the orderings, but I have no idea what i'm doing when it comes to multithreading, so I played it safe 🙂
---
### Author:  [@Matthew-S](https://users.rust-lang.org/u/Matthew-S)
#### Post date: [April 5, 2019, 1:15am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/11 "2019-04-05T01:15:33Z")
Thanks everyone! That looks a lot less hacky using `Arc` and implementing `Drop`. @17cupsofcoffee As a small adjustment, would it make sense to put the `total_count.fetch_add(1, Ordering::SeqCst)` in the `::new()` function to prevent `{ Client::new(...); }` from breaking `total_count`?
---
### Author:  [@17cupsofcoffee](https://users.rust-lang.org/u/17cupsofcoffee)
#### Post date: [April 5, 2019, 6:54am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/12 "2019-04-05T06:54:43Z")
Yeah, that might be a good idea, as otherwise if you dropped your Client before running it, you'd end up with the counter out of sync. Good spot, hadn't considered that 🙂
---
### Author:  [@system](https://users.rust-lang.org/u/system)
#### Post date: [July 4, 2019, 7:08am UTC](https://users.rust-lang.org/t/beginner-code-review-of-simple-sockets-server/26720/13 "2019-07-04T07:08:35Z")
This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.