cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.21k stars 1.1k forks source link

`Default` trait implementation for `Opt` shouldn’t have side-effects #261

Closed palant closed 1 day ago

palant commented 3 weeks ago

Describe the bug

As things are right now, Opt::default() on pingora_core::server::configuration::Opt is implemented as an alias for Opt::parse():

https://github.com/cloudflare/pingora/blob/216d8e9d92c75d46cba78da9b167fb41dec91ad7/pingora-core/src/server/configuration/mod.rs#L168-L172

This can cause unexpected side-effects if someone (like me) gets some Pingora options by other means and merely wants to fill up the data. Depending on the command line parameters the application might even shut down.

Pingora info

Pingora version: 0.2.0 or 216d8e9

Steps to reproduce

Consider the following application:

use pingora_core::server::{configuration::{Opt, ServerConf}, Server};

fn main() {
    let server = Server::new_with_opt_and_conf(
        Opt {
            daemon: true,
            ..Default::default()
        },
        ServerConf::default(),
    );
    println!("Server created");
}

Run it using cargo run -- -h or cargo run -- --blub command.

Expected results

The application should be able to create a server regardless of its command line flags and print: “Server created”

Observed results

Using the first command, the application prints command line help which doesn’t apply to the application and exits. Using the first command, it produces the error message “error: Found argument '--blub' which wasn't expected, or isn't valid in this context” and exits. In both cases the message is never printed.

Additional context

The Default trait implementation can be derived automatically here, it will set all the boolean flags to false and the configuration path to None. If a trait-less alias for Opt::parse() is required, it can be provided under a dedicated and unambiguous name like Opt::parse_command_line().