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.
shadowsocks · Jan 7, 2025 · 8416f6d · 8416f6d
1 parent f8fc8a8
commit 8416f6d
Show file tree

Hide file tree

Showing 6 changed files with 130 additions and 150 deletions.
diff --git 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<F>(
     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<Output = ExitCode>,
+    F: Future<Output = ShadowsocksResult>,
 {
     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
@@ -0,0 +1,40 @@
+//! Shadowsocks-specific error encoding.
+
+/// A result with a shadowsocks-specific error.
+pub type ShadowsocksResult<T = ()> = Result<T, ShadowsocksError>;
+
+/// 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
@@ -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
@@ -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<Output = ExitCode>), ExitCode> {
+pub fn create(matches: &ArgMatches) -> ShadowsocksResult<(Runtime, impl Future<Output = ShadowsocksResult>)> {
     #[cfg_attr(not(feature = "local-online-config"), allow(unused_mut))]
     let (config, _, runtime) = {
         let config_path_opt = matches.get_one::<PathBuf>("CONFIG").cloned().or_else(|| {
@@ -594,13 +595,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
         });
 
         let mut service_config = match config_path_opt {
-            Some(ref config_path) => 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<Output = Exi
         trace!("{:?}", service_config);
 
         let mut config = match config_path_opt {
-            Some(cpath) => 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<Output = Exi
         }
 
         if let Some(acl_file) = matches.get_one::<String>("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<Output = Exi
         // DONE READING options
 
         if config.local.is_empty() {
-            eprintln!(
+            return Err(ShadowsocksError::InsufficientParams(format!(
                 "missing `local_address`, consider specifying it by --local-addr command line option, \
                     or \"local_address\" and \"local_port\" in configuration file"
-            );
-            return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into());
+            )));
         }
 
-        if let Err(err) = config.check_integrity() {
-            eprintln!("config integrity check failed, {err}");
-            return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
-        }
+        config
+            .check_integrity()
+            .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("config integrity check failed, {err}")))?;
 
         #[cfg(unix)]
         if matches.get_flag("DAEMONIZE") || matches.get_raw("DAEMONIZE_PID_PATH").is_some() {
@@ -973,10 +957,9 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
 
         #[cfg(unix)]
         if let Some(uname) = matches.get_one::<String>("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<Output = Exi
                     match server_res {
                         // Server future resolved without an error. This should never happen.
                         Ok(..) => {
-                            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<Output = Exi
 /// Program entrance `main`
 #[inline]
 pub fn main(matches: &ArgMatches) -> 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
@@ -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<Output = ExitCode>), ExitCode> {
+pub fn create(matches: &ArgMatches) -> ShadowsocksResult<(Runtime, impl Future<Output = ShadowsocksResult>)> {
     let (config, runtime) = {
         let config_path_opt = matches.get_one::<PathBuf>("CONFIG").cloned().or_else(|| {
             if !matches.contains_id("SERVER_CONFIG") {
@@ -284,13 +285,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
         });
 
         let mut service_config = match config_path_opt {
-            Some(ref config_path) => 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<Output = Exi
         trace!("{:?}", service_config);
 
         let mut config = match config_path_opt {
-            Some(cpath) => 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<Output = Exi
         }
 
         if let Some(acl_file) = matches.get_one::<String>("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<Output = Exi
 
         // DONE reading options
 
-        if config.manager.is_none() {
-            eprintln!(
+        config.manager.as_ref().ok_or_else(|| {
+            ShadowsocksError::InsufficientParams(format!(
                 "missing `manager_address`, consider specifying it by --manager-address command line option, \
                     or \"manager_address\" and \"manager_port\" keys in configuration file"
-            );
-            return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into());
-        }
+            ))
+        })?;
 
-        if let Err(err) = config.check_integrity() {
-            eprintln!("config integrity check failed, {err}");
-            return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
-        }
+        config
+            .check_integrity()
+            .map_err(|err| ShadowsocksError::LoadConfigFailure(format!("config integrity check failed, {err}")))?;
 
         #[cfg(unix)]
         if matches.get_flag("DAEMONIZE") || matches.get_raw("DAEMONIZE_PID_PATH").is_some() {
@@ -491,10 +475,9 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
 
         #[cfg(unix)]
         if let Some(uname) = matches.get_one::<String>("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<Output = Exi
 
         match future::select(server, abort_signal).await {
             // Server future resolved without an error. This should never happen.
-            Either::Left((Ok(..), ..)) => {
-                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<Output = Exi
 /// Program entrance `main`
 #[inline]
 pub fn main(matches: &ArgMatches) -> 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()
+        }
     }
 }