1
\$\begingroup\$

I'm working on my first Rust project of any particular size, a rudimentary IRC bot. So far I've found solutions to most of the problems I've encountered, but something I'm now getting tangled up in as the project grows is my attempts to apply DI/IoC principles for testability. (I'm coming to Rust from PHP, so I may be overapplying PHP best practices here.)

The full repository is here (link to current HEAD), but I'll reproduce some snippets below. I'm currently getting tangled up in the lifetimes of the structs I'm using for IoC purposes, and every struct seems to require a different combination of Box/Rc/RefCell. Is this a case where it's okay for everything to be 'static? My immediate problem is that attempting to register callback functions with the Dispatcher fails because the Dispatcher (by design) outlives the function doing the registering as well as its parent class.

I've tried to trim out all of the irrelevant code for clarity such as 300 lines of Reply matching, but my apologies if I've thrown out some baby with the bathwater. This should all compile and run as written, but if you want to play around with the code, I suggest going to GitHub and following the setup instructions in the readme. It should work out of the box (IRC server and all) with a couple of Docker commands.

Yes, I'm aware that async exists now, and I'll attempt a refactor using async instead of threads at some point.

// lib.rs
use crate::connection::Connect;
use std::cell::RefCell;
use std::io;
use std::net;
use std::rc::Rc;
use std::thread;
use std::time::Duration;

mod client;
mod connection;
mod dispatcher;

pub fn run<A: net::ToSocketAddrs>(addr: A) -> io::Result<()> {
    let stream = net::TcpStream::connect(addr).expect("Could not connect to server.");

    let dispatcher = Rc::new(RefCell::new(dispatcher::Dispatcher::new()));

    let connection = Rc::new(RefCell::new(connection::Connection::new(
        &stream,
        dispatcher.clone(),
    )));

    let client = client::Client::new(connection.clone(), dispatcher.clone());

    loop {
        if connection.borrow_mut().poll() {
            continue;
        }
        thread::sleep(Duration::from_millis(100));
    }

    //Ok(())
}

Since the IRC protocol is asynchronous, a dispatcher is used to notify interested parties about events received from the server. This could either be commands like incoming messages, or replies to commands sent by the client (not shown).

// dispatcher.rs
use crate::connection;
use std::collections::HashMap;

pub trait Dispatch {
    fn register_command_listener(
        &mut self,
        command_type: connection::CommandType,
        command_listener: Box<dyn Fn(&connection::Command)>,
    );

    fn handle_command(&mut self, command: connection::Command);
}

pub struct Dispatcher {
    command_listeners: HashMap<connection::CommandType, Vec<Box<dyn Fn(&connection::Command)>>>,
}

impl Dispatcher {
    pub fn new() -> Dispatcher {
        Dispatcher {
            command_listeners: HashMap::new(),
        }
    }
}

impl Dispatch for Dispatcher {
    fn register_command_listener(
        &mut self,
        command_type: connection::CommandType,
        command_listener: Box<dyn Fn(&connection::Command)>,
    ) {
        self.command_listeners
            .entry(command_type)
            .or_insert(Vec::new())
            .push(command_listener);
    }

    fn handle_command(&mut self, command: connection::Command) {
        let command_type = command.to_command_type();

        for command_listener in self.command_listeners.entry(command_type).or_default() {
            command_listener(&command);
        }
    }
}

The Connection translates raw strings received over the TCP connection into instances of the Command or Reply (not shown) enums, then sends them for the Dispatcher to handle.

// connection.rs
use crate::dispatcher;
use std::cell::RefCell;
use std::io;
use std::io::prelude::*;
use std::net;
use std::rc::Rc;

pub trait Connect {
    fn poll(&mut self) -> bool;

    fn send_command(&mut self, command: Command) -> std::io::Result<()>;
}

pub struct Connection<'a> {
    reader: Box<dyn 'a + io::BufRead>,
    writer: Box<dyn 'a + Write>,
    dispatcher: Rc<RefCell<dyn 'a + dispatcher::Dispatch>>,
}

impl<'a> Connection<'a> {
    pub fn new(
        stream: &'a net::TcpStream,
        dispatcher: Rc<RefCell<dispatcher::Dispatcher>>,
    ) -> Connection<'a> {
        stream.set_nonblocking(true).unwrap();

        let reader = io::BufReader::new(stream);

        Connection {
            reader: Box::new(reader),
            writer: Box::new(stream),
            dispatcher: dispatcher,
        }
    }

    fn dispatch_message(&mut self, mut raw_message: String) {
        let mut dispatcher = self.dispatcher.borrow_mut();

        if let Some(command) = raw_to_command(&raw_message) {
            dispatcher.handle_command(command);
        }
    }
}

impl<'a> Connect for Connection<'a> {
    fn poll(&mut self) -> bool {
        let mut buffer = String::new();

        match self.reader.read_line(&mut buffer) {
            Ok(len) => {
                if len == 0 {
                    panic!("Stream disconnected");
                } else {
                    print!("< {}", buffer);
                    self.dispatch_message(buffer);
                    true
                }
            }
            Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => false,
            Err(e) => panic!("IO error: {}", e),
        }
    }

    fn send_command(&mut self, command: Command) -> std::io::Result<()> {
        let mut raw_command = command_to_raw(command);
        raw_command.push_str("\r\n");
        print!("> {}", raw_command);
        self.writer.write(raw_command.as_bytes())?;
        Ok(())
    }
}

fn raw_to_command(raw_command: &str) -> Option<Command> {
    let command_parts: Vec<&str> = raw_command.split(' ').collect();

    match command_parts.first()?.as_ref() {
        "PING" => Some(Command::Ping { server: command_parts.get(1)?.to_string() }),
        "PONG" => Some(Command::Pong { server: command_parts.get(1)?.to_string() }),
        // snip
        _ => None,
    }
}

fn command_to_raw(command: Command) -> String {
    match command {
        Command::Ping { server } => format!("PING {}", server),
        Command::Pong { server } => format!("PONG {}", server),
        // snip
    }
}

#[derive(Debug)]
pub enum Command {
    Ping { server: String },
    Pong { server: String },
    // snip
}

impl Command {
    pub fn to_command_type(&self) -> CommandType {
        match self {
            Command::Ping { .. } => CommandType::Ping,
            Command::Pong { .. } => CommandType::Pong,
            // snip
        }
    }
}

#[derive(Hash, Eq, PartialEq, Debug)]
pub enum CommandType {
    Ping,
    Pong,
    // snip
}

The Client understands the semantic meaning of the Command/Reply enums, and will provide methods like send_message_to(user, message), which is translated into a Command instance and sent on to the server. It will also be responsible for interpreting protocol-level messages from the Server, eg. responding to PING and maintaining an up-to-date list of the users in a channel.

// client.rs
use crate::connection;
use crate::dispatcher;
use std::cell::RefCell;
use std::rc::Rc;

pub struct Client<'a> {
    connection: Rc<RefCell<dyn 'a + connection::Connect>>,
    dispatcher: Rc<RefCell<dyn 'a + dispatcher::Dispatch>>,
}

impl<'a> Client<'a> {
    pub fn new(
        connection: Rc<RefCell<connection::Connection<'a>>>,
        dispatcher: Rc<RefCell<dyn 'a + dispatcher::Dispatch>>,
    ) -> Client<'a> {
        // TODO: register with dispatcher to be notified when a PING is received
        Client {
            connection,
            dispatcher,
        }
    }

    fn pong(&self, command: &connection::Command) {
        if let connection::Command::Ping { server } = command {
            self.connection
                .borrow_mut()
                .send_command(connection::Command::Pong {
                    server: "Me".to_string(),
                })
                .ok();
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$
pub trait Dispatch {
    fn register_command_listener(
        &mut self,
        command_type: connection::CommandType,
        command_listener: Box<dyn Fn(&connection::Command)>,
    );

    fn handle_command(&mut self, command: connection::Command);
}

You define a few traits like this. Generally, we don't do this in Rust. Just use the concrete types. So, have references to the Dispatcher struct in your code and not the Dispatch trait.

pub struct Connection<'a> {
    reader: Box<dyn 'a + io::BufRead>,
    writer: Box<dyn 'a + Write>,
    dispatcher: Rc<RefCell<dyn 'a + dispatcher::Dispatch>>,
}

The use of the dyn is slightly inefficient relative to using generics. Generics would also handle lifetimes a bit more easily due to type inference.

fn poll(&mut self) -> bool {
    let mut buffer = String::new();

    match self.reader.read_line(&mut buffer) {
        Ok(len) => {
            if len == 0 {
                panic!("Stream disconnected");
            } else {
                print!("< {}", buffer);
                self.dispatch_message(buffer);
                true
            }
        }
        Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => false,
        Err(e) => panic!("IO error: {}", e),
    }
}

This work by registering and calling function handlers. That's common in other languages, but not the best solution in Rust. What you want to do is return an Option<Command> from this function instead of calling dispatch_message. The calling code can than match on the returned value to decide how to respond to it.

let dispatcher = Rc::new(RefCell::new(dispatcher::Dispatcher::new()));

let connection = Rc::new(RefCell::new(connection::Connection::new(
    &stream,
    dispatcher.clone(),
)));

Rc and RefCell are escape hatches that let you avoid the borrow checker. Using them is usually a sign that your design is insufficiently rusty. In this case, you should have Connection take ownership of Dispatcher and then have Client take ownership of Connection. Then you don't need the Rc/RefCell.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review! If my understanding is correct, using concrete types would prevent me from mocking classes, would it not? Is there a different pattern that I should be using to support unit testing? The Dispatcher (and the poll method) are intended to implement the observer pattern, which is why it requires multiple mutable ownership. Insofar as the Client is the public interface of the module, I suppose it could expose the callback logic on behalf of the Dispatcher, but it involves a lot of boilerplate to bubble up that functionality. \$\endgroup\$ – Mikkel Aug 26 '20 at 2:14
  • \$\begingroup\$ @Mikkel, don't mock classes, don't implement the observer pattern. Those are bad habits you learned in inferior languages. \$\endgroup\$ – Winston Ewert Aug 26 '20 at 4:28
  • \$\begingroup\$ @Mikkel, as it stands, you don't appear to have any tests that use mocks and you don't have anything actually registering with the dispatcher. So its a bit tricky to give concrete advice on what to do instead. \$\endgroup\$ – Winston Ewert Aug 26 '20 at 4:31
  • \$\begingroup\$ @Mikkel, on testing I can give some general notes: Rust's powerful type system makes some tests simply unnecessary. Mocking is overrated, unless it is unbearably slow just use the real implementation. \$\endgroup\$ – Winston Ewert Aug 26 '20 at 4:38
  • 1
    \$\begingroup\$ @Mikkel, in this case, I'd use a substitute implementation of Read/Write. Your Connection code is already holding a dyn io::BufRead and dyn io::Write, so you can simply allow passing in a custom Read and Write. In particular, you might like to use the pipe crate which allow the creation of paired Read/Writes that talk to each other via memory. \$\endgroup\$ – Winston Ewert Sep 2 '20 at 13:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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