From 8416f6dab85b869e574432be9c3dd19be303e39f Mon Sep 17 00:00:00 2001 From: ClaytonKnittel <35512940+ClaytonKnittel@users.noreply.github.com> Date: Tue, 7 Jan 2025 05:25:30 -0600 Subject: [PATCH] refactor: change error handling to be strongly-typed (#1834) * Refactor error handling to be strongly-typed. * Remove unused function. * Remove comment. * Add module + type docstrings. * Make error module private. * Fix windows build. --- bin/sswinservice.rs | 14 ++++---- src/error.rs | 40 ++++++++++++++++++++++ src/lib.rs | 12 +------ src/service/local.rs | 66 ++++++++++++++---------------------- src/service/manager.rs | 76 ++++++++++++++++-------------------------- src/service/server.rs | 72 +++++++++++++++------------------------ 6 files changed, 130 insertions(+), 150 deletions(-) create mode 100644 src/error.rs diff --git a/bin/sswinservice.rs b/bin/sswinservice.rs index 07aef2187780..2cf8d6b2d1b2 100644 --- a/bin/sswinservice.rs +++ b/bin/sswinservice.rs @@ -1,14 +1,16 @@ use std::{ ffi::OsString, future::Future, - process::ExitCode, sync::atomic::{AtomicU32, Ordering}, time::Duration, }; use clap::Command; use log::{error, info}; -use shadowsocks_rust::service::{local, manager, server}; +use shadowsocks_rust::{ + error::ShadowsocksResult, + service::{local, manager, server}, +}; use tokio::{runtime::Runtime, sync::oneshot}; use windows_service::{ define_windows_service, @@ -53,11 +55,11 @@ fn set_service_status( fn handle_create_service_result( status_handle: ServiceStatusHandle, - create_service_result: Result<(Runtime, F), ExitCode>, + create_service_result: ShadowsocksResult<(Runtime, F)>, stop_receiver: oneshot::Receiver<()>, ) -> Result<(), windows_service::Error> where - F: Future, + F: Future, { match create_service_result { Ok((runtime, main_fut)) => { @@ -101,8 +103,8 @@ where Duration::default(), )?; } - Err(exit_code) => { - error!("failed to create service, exit code: {:?}", exit_code); + Err(err) => { + error!("failed to create service, exit code: {:?}", err.exit_code()); // Report running state set_service_status( diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 000000000000..c0d7b170e1a1 --- /dev/null +++ b/src/error.rs @@ -0,0 +1,40 @@ +//! Shadowsocks-specific error encoding. + +/// A result with a shadowsocks-specific error. +pub type ShadowsocksResult = Result; + +/// A generic error class which encodes all possible ways the application can +/// fail, along with debug information. +#[derive(Clone, Debug)] +pub enum ShadowsocksError { + ServerExitUnexpectedly(String), + ServerAborted(String), + LoadConfigFailure(String), + LoadAclFailure(String), + InsufficientParams(String), +} + +impl ShadowsocksError { + /// The corresponding `sysexits::ExitCode` for this error. + pub fn exit_code(&self) -> sysexits::ExitCode { + match self { + Self::ServerExitUnexpectedly(_) | Self::ServerAborted(_) => sysexits::ExitCode::Software, + Self::LoadConfigFailure(_) | Self::LoadAclFailure(_) => sysexits::ExitCode::Config, + Self::InsufficientParams(_) => sysexits::ExitCode::Usage, + } + } +} + +impl std::fmt::Display for ShadowsocksError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::ServerExitUnexpectedly(msg) + | Self::ServerAborted(msg) + | Self::LoadConfigFailure(msg) + | Self::LoadAclFailure(msg) + | Self::InsufficientParams(msg) => write!(f, "{msg}"), + } + } +} + +impl std::error::Error for ShadowsocksError {} diff --git a/src/lib.rs b/src/lib.rs index 13a947b051e0..1f92950bc30b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,6 +4,7 @@ pub mod allocator; pub mod config; #[cfg(unix)] pub mod daemonize; +pub mod error; #[cfg(feature = "logging")] pub mod logging; pub mod monitor; @@ -12,17 +13,6 @@ pub mod service; pub mod sys; pub mod vparser; -/// Exit code when server exits unexpectedly -pub const EXIT_CODE_SERVER_EXIT_UNEXPECTEDLY: sysexits::ExitCode = sysexits::ExitCode::Software; -/// Exit code when server aborted -pub const EXIT_CODE_SERVER_ABORTED: sysexits::ExitCode = sysexits::ExitCode::Software; -/// Exit code when loading configuration from file fails -pub const EXIT_CODE_LOAD_CONFIG_FAILURE: sysexits::ExitCode = sysexits::ExitCode::Config; -/// Exit code when loading ACL from file fails -pub const EXIT_CODE_LOAD_ACL_FAILURE: sysexits::ExitCode = sysexits::ExitCode::Config; -/// Exit code when insufficient params are passed via CLI -pub const EXIT_CODE_INSUFFICIENT_PARAMS: sysexits::ExitCode = sysexits::ExitCode::Usage; - /// Build timestamp in UTC pub const BUILD_TIME: &str = build_time::build_time_utc!(); diff --git a/src/service/local.rs b/src/service/local.rs index a3752fdb7cad..85d7fe3996a8 100644 --- a/src/service/local.rs +++ b/src/service/local.rs @@ -40,6 +40,7 @@ use shadowsocks_service::{ use crate::logging; use crate::{ config::{Config as ServiceConfig, RuntimeMode}, + error::{ShadowsocksError, ShadowsocksResult}, monitor, vparser, }; @@ -576,7 +577,7 @@ pub fn define_command_line_options(mut app: Command) -> Command { } /// Create `Runtime` and `main` entry -pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future), ExitCode> { +pub fn create(matches: &ArgMatches) -> ShadowsocksResult<(Runtime, impl Future)> { #[cfg_attr(not(feature = "local-online-config"), allow(unused_mut))] let (config, _, runtime) = { let config_path_opt = matches.get_one::("CONFIG").cloned().or_else(|| { @@ -594,13 +595,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future match ServiceConfig::load_from_file(config_path) { - Ok(c) => c, - Err(err) => { - eprintln!("loading config {config_path:?}, {err}"); - return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into()); - } - }, + Some(ref config_path) => ServiceConfig::load_from_file(config_path) + .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {config_path:?}, {err}")))?, None => ServiceConfig::default(), }; service_config.set_options(matches); @@ -618,13 +614,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future match Config::load_from_file(&cpath, ConfigType::Local) { - Ok(cfg) => cfg, - Err(err) => { - eprintln!("loading config {cpath:?}, {err}"); - return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into()); - } - }, + Some(cpath) => Config::load_from_file(&cpath, ConfigType::Local) + .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {cpath:?}, {err}")))?, None => Config::new(ConfigType::Local), }; @@ -892,13 +883,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future("ACL") { - let acl = match AccessControl::load_from_file(acl_file) { - Ok(acl) => acl, - Err(err) => { - eprintln!("loading ACL \"{acl_file}\", {err}"); - return Err(crate::EXIT_CODE_LOAD_ACL_FAILURE.into()); - } - }; + let acl = AccessControl::load_from_file(acl_file) + .map_err(|err| ShadowsocksError::LoadAclFailure(format!("loading ACL \"{acl_file}\", {err}")))?; config.acl = Some(acl); } @@ -953,17 +939,15 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future Result<(Runtime, impl Future("USER") { - if let Err(err) = crate::sys::run_as_user(uname) { - eprintln!("failed to change as user, error: {err}"); - return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into()); - } + crate::sys::run_as_user(uname).map_err(|err| { + ShadowsocksError::InsufficientParams(format!("failed to change as user, error: {err}")) + })?; } info!("shadowsocks local {} build {}", crate::VERSION, crate::BUILD_TIME); @@ -1031,19 +1014,17 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future { - eprintln!("server exited unexpectedly"); - return crate::EXIT_CODE_SERVER_EXIT_UNEXPECTEDLY.into(); + return Err(ShadowsocksError::ServerExitUnexpectedly("server exited unexpectedly".to_owned())); } // Server future resolved with error, which are listener errors in most cases Err(err) => { - eprintln!("server aborted with {err}"); - return crate::EXIT_CODE_SERVER_ABORTED.into(); + return Err(ShadowsocksError::ServerAborted(format!("server aborted with {err}"))); } } } // The abort signal future resolved. Means we should just exit. _ = abort_signal => { - return ExitCode::SUCCESS; + return Ok(()); } _ = reload_task => { // continue. @@ -1059,9 +1040,12 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future ExitCode { - match create(matches) { - Ok((runtime, main_fut)) => runtime.block_on(main_fut), - Err(code) => code, + match create(matches).and_then(|(runtime, main_fut)| runtime.block_on(main_fut)) { + Ok(()) => ExitCode::SUCCESS, + Err(err) => { + eprintln!("{err}"); + err.exit_code().into() + } } } diff --git a/src/service/manager.rs b/src/service/manager.rs index 2cd5a861fc0e..0bb2b55fd3fa 100644 --- a/src/service/manager.rs +++ b/src/service/manager.rs @@ -27,6 +27,7 @@ use shadowsocks_service::{ use crate::logging; use crate::{ config::{Config as ServiceConfig, RuntimeMode}, + error::{ShadowsocksError, ShadowsocksResult}, monitor, vparser, }; @@ -267,7 +268,7 @@ pub fn define_command_line_options(mut app: Command) -> Command { } /// Create `Runtime` and `main` entry -pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future), ExitCode> { +pub fn create(matches: &ArgMatches) -> ShadowsocksResult<(Runtime, impl Future)> { let (config, runtime) = { let config_path_opt = matches.get_one::("CONFIG").cloned().or_else(|| { if !matches.contains_id("SERVER_CONFIG") { @@ -284,13 +285,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future match ServiceConfig::load_from_file(config_path) { - Ok(c) => c, - Err(err) => { - eprintln!("loading config {config_path:?}, {err}"); - return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into()); - } - }, + Some(ref config_path) => ServiceConfig::load_from_file(config_path) + .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {config_path:?}, {err}")))?, None => ServiceConfig::default(), }; service_config.set_options(matches); @@ -308,13 +304,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future match Config::load_from_file(&cpath, ConfigType::Manager) { - Ok(cfg) => cfg, - Err(err) => { - eprintln!("loading config {cpath:?}, {err}"); - return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into()); - } - }, + Some(cpath) => Config::load_from_file(&cpath, ConfigType::Manager) + .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {cpath:?}, {err}")))?, None => Config::new(ConfigType::Manager), }; @@ -421,13 +412,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future("ACL") { - let acl = match AccessControl::load_from_file(acl_file) { - Ok(acl) => acl, - Err(err) => { - eprintln!("loading ACL \"{acl_file}\", {err}"); - return Err(crate::EXIT_CODE_LOAD_ACL_FAILURE.into()); - } - }; + let acl = AccessControl::load_from_file(acl_file) + .map_err(|err| ShadowsocksError::LoadAclFailure(format!("loading ACL \"{acl_file}\", {err}")))?; config.acl = Some(acl); } @@ -470,18 +456,16 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future Result<(Runtime, impl Future("USER") { - if let Err(err) = crate::sys::run_as_user(uname) { - eprintln!("failed to change as user, error: {err}"); - return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into()); - } + crate::sys::run_as_user(uname).map_err(|err| { + ShadowsocksError::InsufficientParams(format!("failed to change as user, error: {err}")) + })?; } info!("shadowsocks manager {} build {}", crate::VERSION, crate::BUILD_TIME); @@ -526,17 +509,13 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future { - eprintln!("server exited unexpectedly"); - crate::EXIT_CODE_SERVER_EXIT_UNEXPECTEDLY.into() - } + Either::Left((Ok(..), ..)) => Err(ShadowsocksError::ServerExitUnexpectedly( + "server exited unexpectedly".to_owned(), + )), // Server future resolved with error, which are listener errors in most cases - Either::Left((Err(err), ..)) => { - eprintln!("server aborted with {err}"); - crate::EXIT_CODE_SERVER_ABORTED.into() - } + Either::Left((Err(err), ..)) => Err(ShadowsocksError::ServerAborted(format!("server aborted with {err}"))), // The abort signal future resolved. Means we should just exit. - Either::Right(_) => ExitCode::SUCCESS, + Either::Right(_) => Ok(()), } }; @@ -546,9 +525,12 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future ExitCode { - match create(matches) { - Ok((runtime, main_fut)) => runtime.block_on(main_fut), - Err(code) => code, + match create(matches).and_then(|(runtime, main_fut)| runtime.block_on(main_fut)) { + Ok(()) => ExitCode::SUCCESS, + Err(err) => { + eprintln!("{err}"); + err.exit_code().into() + } } } diff --git a/src/service/server.rs b/src/service/server.rs index ad30773b652c..a905bec767b9 100644 --- a/src/service/server.rs +++ b/src/service/server.rs @@ -25,6 +25,7 @@ use shadowsocks_service::{ use crate::logging; use crate::{ config::{Config as ServiceConfig, RuntimeMode}, + error::{ShadowsocksError, ShadowsocksResult}, monitor, vparser, }; @@ -279,7 +280,7 @@ pub fn define_command_line_options(mut app: Command) -> Command { } /// Create `Runtime` and `main` entry -pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future), ExitCode> { +pub fn create(matches: &ArgMatches) -> ShadowsocksResult<(Runtime, impl Future)> { let (config, runtime) = { let config_path_opt = matches.get_one::("CONFIG").cloned().or_else(|| { if !matches.contains_id("SERVER_CONFIG") { @@ -296,13 +297,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future match ServiceConfig::load_from_file(config_path) { - Ok(c) => c, - Err(err) => { - eprintln!("loading config {config_path:?}, {err}"); - return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into()); - } - }, + Some(ref config_path) => ServiceConfig::load_from_file(config_path) + .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {config_path:?}, {err}")))?, None => ServiceConfig::default(), }; service_config.set_options(matches); @@ -320,13 +316,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future match Config::load_from_file(&cpath, ConfigType::Server) { - Ok(cfg) => cfg, - Err(err) => { - eprintln!("loading config {cpath:?}, {err}"); - return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into()); - } - }, + Some(cpath) => Config::load_from_file(&cpath, ConfigType::Server) + .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {cpath:?}, {err}")))?, None => Config::new(ConfigType::Server), }; @@ -445,13 +436,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future("ACL") { - let acl = match AccessControl::load_from_file(acl_file) { - Ok(acl) => acl, - Err(err) => { - eprintln!("loading ACL \"{acl_file}\", {err}"); - return Err(crate::EXIT_CODE_LOAD_ACL_FAILURE.into()); - } - }; + let acl = AccessControl::load_from_file(acl_file) + .map_err(|err| ShadowsocksError::LoadAclFailure(format!("loading ACL \"{acl_file}\", {err}")))?; config.acl = Some(acl); } @@ -495,18 +481,16 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future Result<(Runtime, impl Future("USER") { - if let Err(err) = crate::sys::run_as_user(uname) { - eprintln!("failed to change as user, error: {err}"); - return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into()); - } + crate::sys::run_as_user(uname).map_err(|err| { + ShadowsocksError::InsufficientParams(format!("failed to change as user, error: {err}")) + })?; } info!("shadowsocks server {} build {}", crate::VERSION, crate::BUILD_TIME); @@ -551,17 +534,13 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future { - eprintln!("server exited unexpectedly"); - crate::EXIT_CODE_SERVER_EXIT_UNEXPECTEDLY.into() - } + Either::Left((Ok(..), ..)) => Err(ShadowsocksError::ServerExitUnexpectedly( + "server exited unexpectedly".to_owned(), + )), // Server future resolved with error, which are listener errors in most cases - Either::Left((Err(err), ..)) => { - eprintln!("server aborted with {err}"); - crate::EXIT_CODE_SERVER_ABORTED.into() - } + Either::Left((Err(err), ..)) => Err(ShadowsocksError::ServerAborted(format!("server aborted with {err}"))), // The abort signal future resolved. Means we should just exit. - Either::Right(_) => ExitCode::SUCCESS, + Either::Right(_) => Ok(()), } }; @@ -571,9 +550,12 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future ExitCode { - match create(matches) { - Ok((runtime, main_fut)) => runtime.block_on(main_fut), - Err(code) => code, + match create(matches).and_then(|(runtime, main_fut)| runtime.block_on(main_fut)) { + Ok(()) => ExitCode::SUCCESS, + Err(err) => { + eprintln!("{err}"); + err.exit_code().into() + } } }