minitalk project using Rust | Communication between process using UNIX Signals

Question

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

Answer 1

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.