2
\$\begingroup\$

I am trying to build minitalk project from 42 school using Rust instead of C. This is a communication program between a client and a server. Both are process. To achieve this I am only allowed to use UNIX Signals SIGUSR1 and SIGUSR2. While I got help to achieve this, I would appreciate an external point of view for the below code.

server.rs

use signal_hook::{iterator::Signals, consts::{SIGUSR1, SIGUSR2}};
use std::error::Error;
use std::sync::{Arc, Mutex};
use std::time::{Instant, Duration};
use std::thread;

fn main() -> Result<(), Box<dyn Error>> {
    let pid = std::process::id();
    println!("Server PID: {}", pid);

    let message = Arc::new(Mutex::new(Vec::new()));
    let last_received = Arc::new(Mutex::new(Instant::now()));
    let mut signals = Signals::new(&[SIGUSR1, SIGUSR2])?;

    let message_clone = Arc::clone(&message);
    let last_received_clone = Arc::clone(&last_received);
    thread::spawn(move || {
        let mut byte = 0;
        let mut bit_count = 0;
        for sig in signals.forever() {
            let mut last_received = last_received_clone.lock().unwrap();
            *last_received = Instant::now();
            drop(last_received);

            let mut message = message_clone.lock().unwrap();
            match sig {
                SIGUSR1 => byte |= 1 << bit_count,
                SIGUSR2 => (),
                _ => unreachable!(),
            }
            bit_count += 1;
            if bit_count == 8 {
                message.push(byte);
                byte = 0;
                bit_count = 0;
            }
        }
    });

    loop {
        thread::sleep(Duration::from_millis(1));
        let last_received = last_received.lock().unwrap();
        if last_received.elapsed() >= Duration::from_millis(50) {
            let mut message = message.lock().unwrap();
            if !message.is_empty() {
                let message_str = String::from_utf8_lossy(&message);
                println!("{}", message_str);
                message.clear();
            }
        }
    }
}

client.rs

use std::process::Command;
use std::env;

fn main() {
    let args: Vec<String> = env::args().collect();
    let pid = &args[1];
    let message = &args[2];

    for byte in message.bytes() {
        for i in 0..8 {
            let bit = (byte >> i) & 1;
            let signal = if bit == 1 { "SIGUSR1" } else { "SIGUSR2" };
            Command::new("kill")
                .arg(format!("-{}", signal))
                .arg(pid)
                .output()
                .expect("Failed to send signal");
        }
    }
}

Cargo.toml

[package]
name = "minitalk"
version = "0.1.0"
edition = "2021"

[dependencies]
nix = "0.27.1"
signal-hook = "0.3.17"
libc = "0.2"

[[bin]]
name = "server"
path = "./src/server.rs"

[[bin]]
name = "client"
path = "./src/client.rs"

Execution: Open two terminal inside src directory (one to execute the server, one for client).

1st terminal: cargo run --bin server Server PID: "{x}"

2nd terminal: cargo run --bin client server_pid hello

hello will print on the stdout of server bin.

While this work as expected, I would like to have your opinion on this program as this is in Rust and not C. Thanks

\$\endgroup\$
1
  • \$\begingroup\$ Not worth a whole review but if you put server.rs and client.rs into src/bin you don't need the sections in your Cargo.toml because you conform to the standard project layout \$\endgroup\$
    – cafce25
    Sep 27, 2023 at 10:32

1 Answer 1

2
\$\begingroup\$

This code is racy. It does not appear capable of reliably sending a bytestream.


map

            let signal = if bit == 1 { "SIGUSR1" } else { "SIGUSR2" };

That if expression is nice enough. But consider using a HashMap instead.


syscall vs spawn a child

            Command::new("kill")
                .arg(format!("-{}", signal))
                .arg(pid)

zomg, that fork + exec seems expensive. Just send a signal from within the current process.

Of course, "expensive" might actually be a good thing, as the delay masks other issues, and gives the scheduler a chance to run. I was expecting to see a call to signal() followed by a call to sleep(), giving the server a chance to receive the signal. Or, much better, a call to acquire a mutex or manipulate a condition variable so we know the server is ready to receive the next bit.


delete dead code

    let last_received_clone = Arc::clone(&last_received);

Those two variables appear to be related to some "display a timestamp" debugging code that was deleted prior to review. Now that you're getting ready to merge to main, it is the right time to tidy up vestiges of debugs.


magic numbers

        thread::sleep(Duration::from_millis(1));
        ...
        if last_received.elapsed() >= Duration::from_millis(50) {

The 1 is for yielding control to the scheduler, fine. Maybe 0 would suffice.

The 50 needs a name and an explanation. I imagine that on a certain host it allows server to mostly win the races. It's unclear how it was arrived at, and how one would choose an appropriate constant on a Cray or a Raspberry Pi.


race

Here is the heart of the issue:

        for sig in signals.forever() {

The trouble is that on unix / linux systems, there's no queueing of signals. For example client might send a pair of SIGUSR1's, yet just a single SIGUSR1 is picked up in this loop. The OS is allowed to coalesce rapidly sent signals.

How to fix?

  1. Use IPC that makes appropriate guarantees, such as a muxtex, or
  2. have client await a handshake signal before sending its next signal.

For the current (racy) code, consider appending a validation checksum. Or allow re-sync like a UART does by sending start + stop bits. Or send redundant FEC data.


This code does not yet appear to achieve its design goals.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.