vorner / spirit

Apache License 2.0
162 stars 6 forks source link

Start a user guide #43

Closed Michael-F-Bryan closed 4 years ago

Michael-F-Bryan commented 5 years ago

This is the first step in the path towards #42.

Michael-F-Bryan commented 5 years ago

@vorner we'll probably want to build the user guide and push to GitHub Pages on every commit to master so I've updated travis accordingly (see the second commit). You'll need to create a Personal Access Token and tell travis to include it as the $GITHUB_TOKEN environment variable. Full instructions are here.

Michael-F-Bryan commented 5 years ago

Thanks for reviewing this @vorner. Because I'm tweaking CI to build/deploy the user guide I tend to make lots of little commits so you can see the effect of a change, then when we're happy with the CI setup it can all be squashed to clean up history.

Would it be possible to split the build of the mdbook and publishing into a separate job in a phase after the normal tests?

I'll see if we can download the binary from github releases instead of installing it from source. That should improve things.

vorner commented 5 years ago

Cleaning up when it's ready sounds fine, yes.

I'm not really a fan of curl | sh installs, but I guess it's not my machine it's running on, so that's probably fine.

Currently, the CI fails on nightly with rustc panic (index out of bounds). So we'll have to wait for it to get fixed there. However, the changes as I read them look good to me :+1: ‒ I'll merge it once the CI turns green, I'll be checking it from time to time to see if it's fixed. Or, can you do the cleanup first? I hope there won't be more changes needed.

Michael-F-Bryan commented 5 years ago

The CI failure looks like an ICE in nightly rustdoc (rust-lang/rust#65840) and nothing to do with the user guide.

I'm going to squash the CI changes and start actually writing the user guide now. The API docs explain the various pieces (Fragment, Builder, etc.) quite well so the user guide can focus on implementing a hello world TCP service and provide links to the API docs if people want to read more.

My rough plan:

  1. Give a brief overview of the problem spirit is trying to solve (taken mostly from existing blog posts)
  2. Introduce a basic hello world TCP server which we're going to turn into a daemon/service
  3. Wire up the Builder so our TCP server runs, adding the bare minimum in terms of configuration or command-line arguments
  4. Introduce configuration and reloading with SIGHUP (may touch on TCP port reloading, but cleanly changing ports/connections could be hard)
  5. Wire up logging
  6. Add metrics to record how many people use our service
  7. Installing the program as a system service (systemd, spirit-daemonize, etc.)
  8. Describe the role of various helper crates? (spirit-hyper, spirit-reqwest, spirit-tokio, etc.)
vorner commented 5 years ago

It sounds good in general. Just, I'm a bit confused here:

Introduce configuration and reloading with SIGHUP (may touch on TCP port reloading, but cleanly changing ports/connections could be hard)

You should get this from spirit for free, including the changing of ports.

Do you want to develop the guide in this PR, or open multiple and get it in in parts?

Michael-F-Bryan commented 5 years ago

You should get this from spirit for free, including the changing of ports.

Yeah, this is more about showing the commands you can use to change configuration and tell the server to reload, and touching on the things spirit is doing for you in the background.

I wasn't sure how the synchronisation of TCP connections would work, but I guess if your code looks

.run(|spirit| {
  while !spirit.is_terminated() {
    let conn = server.accept()?;
    std::thread::spawn(move || handle_connection(conn));
  }
  Ok(())
})

Then the server will start listening on the new port and any threads handling connections on the old port will just run to completion. So the user's code doesn't really need to worry about synchronisation steps like killing old connections or joining all threads, do they?

Do you want to develop the guide in this PR, or open multiple and get it in in parts?

It might be easier to do open individual PRs for each topic, that way it's easier to review and we're not trying to merge the entire thing in one PR. It'd also make it easier for other people to join in and contribute.

vorner commented 5 years ago

When talking about ports, I thought we are talking about the stuff in spirit-tokio. That one handles changing the listening ports automagically in the background ‒ it creates a new listener as needed, shuts down the old if it works, but lets the previous connections to continue.

The code you posted will not adapt to new ports. The is_terminated is about the final termination, not about changing configuration, so there's nothing that could take that server from you and change it under your hands. In case of the std::net::TcpListen, some kind of new sub-crate would need to be written or handled in user code somehow ‒ but I don't expect many „big“ applications that use spirit to use the networking primitives from std.

There's also another downside to the code as you wrote it ‒ if no new connections are coming, it will not terminate, because it'll block on the call to accept. You'd need some way to either close the socket from the terminate callback (which is technically possible, but probably inconvenient) or set some timeout on the accept (which I can't find, currently :-().

vorner commented 5 years ago

I wonder how long this https://github.com/rust-lang/rust/issues/65840 will take to resolve (btw, thanks for reporting it, I was being lazy about it). Do you think we should go with some kind of backup plan? Mark nightly as allowed failure + take the docs from stable branch instead? I guess we lose the intra-doc links, though :-(, but we could proceed and switch back to nightly once that is resolved.

vorner commented 5 years ago

Ping? I What's your opinion on using stable for now, until the issue is fixed? It would be great to move forward here.

vorner commented 5 years ago

Thank you. Can you also squash the last few commits that deal with the CI? I'll merge it later today.

Michael-F-Bryan commented 4 years ago

Sure thing. I've cleaned up the history and reworded the overview to take your comments into account.

I've also been writing a couple daemons to get more familiar with spirit. I wrote a simple daemon which will periodically print a message to STDOUT, would you say this is a fairly correct way to write a daemon using spirit?

A simple daemon which will print out a message at a specified interval ```rust //! A simple daemon which will print out a message at a specified interval. use arc_swap::ArcSwap; use serde::Deserialize; use spirit::{prelude::*, AnyError, Empty, Spirit}; use std::{ sync::mpsc::{self, Receiver, RecvTimeoutError}, time::Duration, }; #[derive(Debug, Clone, PartialEq, Deserialize)] struct Config { /// The message to print. message: String, /// The number of seconds between messages. interval: f32, } impl Config { fn interval(&self) -> Duration { Duration::from_secs_f32(self.interval) } } impl Default for Config { fn default() -> Config { Config { message: String::from("Hello, World!"), interval: 2.0, } } } const DEFAULT_CONFIG: &str = r#" message = "Hello, World!" interval = 2 "#; #[derive(Debug, Copy, Clone, PartialEq)] enum ControlFlow { Reload, Shutdown, } fn main() -> Result<(), AnyError> { // Here we'll have the current config stored at each time. The spirit // library will refresh it with a new version on reload (and the old one // will get dropped eventually). let config = ArcSwap::from_pointee(Config::default()); // set up a channel for communicating between spirit and the rest of the // application let (tx, rx) = mpsc::channel(); // we need to clone the sender because we're passing it to two callbacks let tx2 = tx.clone(); // start initializing the spirit system let _spirit = Spirit::::new() // Make sure whenever configuration changes, the CONFIG variable is // kept in sync .with(spirit_cfg_helpers::cfg_store(config.clone())) // Set the default config values. This is very similar to passing the // first file on command line, except that nobody can lose this one as // it is baked into the application. Any files passed by the user can // override the values. .config_defaults(DEFAULT_CONFIG) // Tell the server whenever configuration has changed .on_config(move |_, _| tx2.send(ControlFlow::Reload).unwrap()) // Spirit received a terminate signal, propagate it to the server .on_terminate(move || { tx.send(ControlFlow::Shutdown).unwrap(); }) .build(true)?; println!("Starting up"); print_message_indefinitely(config, rx); println!("Shutting down"); Ok(()) } fn print_message_indefinitely(config: ArcSwap, receiver: Receiver) { loop { let interval = config.load().interval(); match receiver.recv_timeout(interval) { Err(RecvTimeoutError::Timeout) => { // We slept for `interval` seconds without Spirit telling us // to stop or reload the configuration. We can print the message! println!("{}", config.load().message); } Ok(ControlFlow::Reload) => { // the configuration has been updated, go back to the start of // the loop and reload the interval println!("Reloading configuration"); continue; } Ok(ControlFlow::Shutdown) => { // We've been told to shutdown. return; } Err(RecvTimeoutError::Disconnected) => { println!("The other end disconnected"); return; } } } } ```
vorner commented 4 years ago

Thanks again, travis is building it now, let's see what happens…

The code you posted would work and is OK I think as it is. I'd however propose some little tweaks there:

I'll try to find some time in the meantime to update readme and the crate api docs to point to the generated documentation.

vorner commented 4 years ago

Also, an alternative (this one logs the message, doesn't print, but that should be little difference). There's the problem this doesn't terminate or reload right away, it waits the rest of the interval to react so it's in a sense inferior. But it shows some other goodies too:

use log::info;
use serde::{Deserialize, Serialize};
use spirit::{Pipeline, Spirit};
use spirit::prelude::*;
use spirit::utils::serialize_duration;
use spirit_cfg_helpers::Opts as CfgOpts;
use spirit_log::{Cfg as LogCfg, CfgAndOpts as LogBoth, Opts as LogOpts};
use structdoc::StructDoc;
use structopt::StructOpt;

use std::thread;
use std::time::Duration;

#[derive(Clone, Debug, StructOpt)]
struct Opts {
    #[structopt(flatten)]
    logging: LogOpts,

    #[structopt(flatten)]
    cfg_opts: CfgOpts,
}

impl Opts {
    fn logging(&self) -> LogOpts {
        self.logging.clone()
    }

    fn cfg_opts(&self) -> &CfgOpts {
        &self.cfg_opts
    }
}

#[derive(Clone, Debug, Default, Deserialize, Serialize, StructDoc)]
struct Cfg {
    #[serde(default, skip_serializing_if = "LogCfg::is_empty")]
    logging: LogCfg,

    /// The message that gets repeatedly logged.
    message: String,

    /// An interval between two messages.
    #[serde(deserialize_with = "serde_humantime::deserialize", serialize_with="serialize_duration")]
    interval: Duration,
}

impl Cfg {
    fn logging(&self) -> LogCfg {
        self.logging.clone()
    }
}

fn main() {
    Spirit::<Opts, Cfg>::new()
        .config_supported_exts()
        .with(CfgOpts::extension(Opts::cfg_opts))
        .with(
            Pipeline::new("logging").extract(|opts: &Opts, cfg: &Cfg| LogBoth {
                cfg: cfg.logging(),
                opts: opts.logging(),
            }),
        )
        .run(|spirit| {
            while !spirit.is_terminated() {
                let cfg = spirit.config();
                thread::sleep(cfg.interval);
                info!("{}", cfg.message);
            }
            Ok(())
        });
}