4
\$\begingroup\$

I'm learning rust by implementing simple use-cases that are common and trivial in other languages. Recently, I decided to prototype a small component for retrieving some "config" from a remote source every X minutes. The local version of config should also be made available to the rest of the program.

This is what I came up with. I'm looking for improvements in my code, especially within the get_config method where I had to clone the entire config. Note that a lot of stuff (like error handling and graceful shutdown) has been left out for brevity.

use std::{sync::{Arc, RwLock}, thread::{self, JoinHandle, sleep}, time::Duration};


#[derive(Clone, Debug)]
struct Config {
    // Simplest possible config. 
    pub quote: String
}

struct ConfigProvider {
    refresher: Option<JoinHandle<()>>,
    config: Arc<RwLock<Config>>
}

impl ConfigProvider {

    pub fn new() -> ConfigProvider {
        let mut provider = ConfigProvider{
            refresher: None,
            config: Arc::new(RwLock::new(Config {quote: "(empty)".to_owned() }))
        };
        let data_ref = Arc::clone(&provider.config);
        let th = thread::spawn(move || ConfigProvider::refresh(data_ref));
        provider.refresher = Some(th);
        provider
    }

    fn get_config(&self) -> Config {
        // Question: How to avoid cloning here, so this method 
        // becomes as cheap as possible ? 
        return self.config.read().unwrap().clone();
    }

    fn refresh(dest: Arc<RwLock<Config>>) {
        loop {
            sleep(Duration::from_secs(5));
            println!("Refreshing quote..");
            let response = reqwest::blocking::get("https://api.quotable.io/random");
            if response.is_err() {
                println!("Error fetching quote: {:?}", response.err().unwrap());
                continue;
            }

            let text = response.unwrap().text();
            if text.is_err() {
                println!("Error reading quote: {:?}", text.err().unwrap());
                continue;
            }

            let quote = text.unwrap();
            {
                let mut config = dest.write().unwrap();
                config.quote = quote;
            }
        }
    }
}


fn main() {
    let provider = ConfigProvider::new();
    loop {
        sleep(Duration::from_secs(3));
        let config = provider.get_config();
        println!("Retrieved quote: {}", config.quote);
    }
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

You can change

config: Arc<RwLock<Config>>

...

fn get_config(&self) -> Config {

to

config: Arc<RwLock<Arc<Config>>>,

...

fn get_config(&self) -> Arc<Config> {

This way, while you do still need to clone() the value out of the lock, the clone simply consists of incrementing the reference count, not copying all data in the Config.

A further refinement of this approach is to use the arc-swap library, which lets you avoid using any locking at all, but update the config by atomically swapping in a new Arc<Config> pointer. This has the advantage that the config updater cannot ever cause the config readers to block. In that case, you'd write

config: ArcSwap<Config>,

and the ArcSwap type handles all the rest of the functionality.


Another thing: Right now, ConfigProvider is structured around both updating the config and handing it out. Notably, it isn't Clone. This may prove inconvenient later, because code which needs the config cannot own something that lets it fetch the latest config. I recommend that you explicitly split out the “sending” role from the “receiving” one, like channels do. Now, in the case of the code you've shown here, there is actually nothing to do but stop storing the JoinHandle:

#[derive(Clone, Debug)]
struct ConfigProvider {
    config: Arc<RwLock<Arc<Config>>>,
}

// or if you take the above advice to use `arc-swap`,
#[derive(Clone, Debug)]
struct ConfigProvider {
    // need an outer Arc to make it shareable by cloning
    config: Arc<ArcSwap<Config>>,
}

Now, any code that wants to track the latest config can clone and hang onto a ConfigProvider indefinitely; it acts as the receiving side of the channel, and the thread acts as the sending side.

\$\endgroup\$
2
\$\begingroup\$

Right now the ConfigProvider mixes the responsibilities of owning the config, scheduling the updates and also performing the actual updates having to know the config structure. If you want to reuse this tool in another project you'd have to copy-n-paste.

I suggest to restructure the solution and provide the ConfigProvider with a generic type parameter that represents the actual config and maybe an update function that can simply update a config instance.

This way the implementation of the actual config and how it receives its values and the scheduling of background updates and keeping it thread-safe is split. Makes it more versatile for the future IMHO.

Update: Also I'd avoid println all over the place - use log and one of the logging implementations to get message output.

\$\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.