Skip to content

Commit 8416f6d

Browse files
ClaytonKnittelClaytonKnittel
authored
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.
1 parent f8fc8a8 commit 8416f6d

File tree

6 files changed

+130
-150
lines changed

6 files changed

+130
-150
lines changed

‎bin/sswinservice.rs‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
11
use std::{
22
ffi::OsString,
33
future::Future,
4-
process::ExitCode,
54
sync::atomic::{AtomicU32, Ordering},
65
time::Duration,
76
};
87

98
use clap::Command;
109
use log::{error, info};
11-
use shadowsocks_rust::service::{local, manager, server};
10+
use shadowsocks_rust::{
11+
error::ShadowsocksResult,
12+
service::{local, manager, server},
13+
};
1214
use tokio::{runtime::Runtime, sync::oneshot};
1315
use windows_service::{
1416
define_windows_service,
@@ -53,11 +55,11 @@ fn set_service_status(
5355

5456
fn handle_create_service_result<F>(
5557
status_handle: ServiceStatusHandle,
56-
create_service_result: Result<(Runtime, F), ExitCode>,
58+
create_service_result: ShadowsocksResult<(Runtime, F)>,
5759
stop_receiver: oneshot::Receiver<()>,
5860
) -> Result<(), windows_service::Error>
5961
where
60-
F: Future<Output = ExitCode>,
62+
F: Future<Output = ShadowsocksResult>,
6163
{
6264
match create_service_result {
6365
Ok((runtime, main_fut)) => {
@@ -101,8 +103,8 @@ where
101103
Duration::default(),
102104
)?;
103105
}
104-
Err(exit_code) => {
105-
error!("failed to create service, exit code: {:?}", exit_code);
106+
Err(err) => {
107+
error!("failed to create service, exit code: {:?}", err.exit_code());
106108

107109
// Report running state
108110
set_service_status(

‎src/error.rs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
//! Shadowsocks-specific error encoding.
2+
3+
/// A result with a shadowsocks-specific error.
4+
pub type ShadowsocksResult<T = ()> = Result<T, ShadowsocksError>;
5+
6+
/// A generic error class which encodes all possible ways the application can
7+
/// fail, along with debug information.
8+
#[derive(Clone, Debug)]
9+
pub enum ShadowsocksError {
10+
ServerExitUnexpectedly(String),
11+
ServerAborted(String),
12+
LoadConfigFailure(String),
13+
LoadAclFailure(String),
14+
InsufficientParams(String),
15+
}
16+
17+
impl ShadowsocksError {
18+
/// The corresponding `sysexits::ExitCode` for this error.
19+
pub fn exit_code(&self) -> sysexits::ExitCode {
20+
match self {
21+
Self::ServerExitUnexpectedly(_) | Self::ServerAborted(_) => sysexits::ExitCode::Software,
22+
Self::LoadConfigFailure(_) | Self::LoadAclFailure(_) => sysexits::ExitCode::Config,
23+
Self::InsufficientParams(_) => sysexits::ExitCode::Usage,
24+
}
25+
}
26+
}
27+
28+
impl std::fmt::Display for ShadowsocksError {
29+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
30+
match self {
31+
Self::ServerExitUnexpectedly(msg)
32+
| Self::ServerAborted(msg)
33+
| Self::LoadConfigFailure(msg)
34+
| Self::LoadAclFailure(msg)
35+
| Self::InsufficientParams(msg) => write!(f, "{msg}"),
36+
}
37+
}
38+
}
39+
40+
impl std::error::Error for ShadowsocksError {}

‎src/lib.rs‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ pub mod allocator;
44
pub mod config;
55
#[cfg(unix)]
66
pub mod daemonize;
7+
pub mod error;
78
#[cfg(feature = "logging")]
89
pub mod logging;
910
pub mod monitor;
@@ -12,17 +13,6 @@ pub mod service;
1213
pub mod sys;
1314
pub mod vparser;
1415

15-
/// Exit code when server exits unexpectedly
16-
pub const EXIT_CODE_SERVER_EXIT_UNEXPECTEDLY: sysexits::ExitCode = sysexits::ExitCode::Software;
17-
/// Exit code when server aborted
18-
pub const EXIT_CODE_SERVER_ABORTED: sysexits::ExitCode = sysexits::ExitCode::Software;
19-
/// Exit code when loading configuration from file fails
20-
pub const EXIT_CODE_LOAD_CONFIG_FAILURE: sysexits::ExitCode = sysexits::ExitCode::Config;
21-
/// Exit code when loading ACL from file fails
22-
pub const EXIT_CODE_LOAD_ACL_FAILURE: sysexits::ExitCode = sysexits::ExitCode::Config;
23-
/// Exit code when insufficient params are passed via CLI
24-
pub const EXIT_CODE_INSUFFICIENT_PARAMS: sysexits::ExitCode = sysexits::ExitCode::Usage;
25-
2616
/// Build timestamp in UTC
2717
pub const BUILD_TIME: &str = build_time::build_time_utc!();
2818

‎src/service/local.rs‎

Lines changed: 25 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ use shadowsocks_service::{
4040
use crate::logging;
4141
use crate::{
4242
config::{Config as ServiceConfig, RuntimeMode},
43+
error::{ShadowsocksError, ShadowsocksResult},
4344
monitor, vparser,
4445
};
4546

@@ -576,7 +577,7 @@ pub fn define_command_line_options(mut app: Command) -> Command {
576577
}
577578

578579
/// Create `Runtime` and `main` entry
579-
pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = ExitCode>), ExitCode> {
580+
pub fn create(matches: &ArgMatches) -> ShadowsocksResult<(Runtime, impl Future<Output = ShadowsocksResult>)> {
580581
#[cfg_attr(not(feature = "local-online-config"), allow(unused_mut))]
581582
let (config, _, runtime) = {
582583
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
594595
});
595596

596597
let mut service_config = match config_path_opt {
597-
Some(ref config_path) => match ServiceConfig::load_from_file(config_path) {
598-
Ok(c) => c,
599-
Err(err) => {
600-
eprintln!("loading config {config_path:?}, {err}");
601-
return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
602-
}
603-
},
598+
Some(ref config_path) => ServiceConfig::load_from_file(config_path)
599+
.map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {config_path:?}, {err}")))?,
604600
None => ServiceConfig::default(),
605601
};
606602
service_config.set_options(matches);
@@ -618,13 +614,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
618614
trace!("{:?}", service_config);
619615

620616
let mut config = match config_path_opt {
621-
Some(cpath) => match Config::load_from_file(&cpath, ConfigType::Local) {
622-
Ok(cfg) => cfg,
623-
Err(err) => {
624-
eprintln!("loading config {cpath:?}, {err}");
625-
return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
626-
}
627-
},
617+
Some(cpath) => Config::load_from_file(&cpath, ConfigType::Local)
618+
.map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {cpath:?}, {err}")))?,
628619
None => Config::new(ConfigType::Local),
629620
};
630621

@@ -892,13 +883,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
892883
}
893884

894885
if let Some(acl_file) = matches.get_one::<String>("ACL") {
895-
let acl = match AccessControl::load_from_file(acl_file) {
896-
Ok(acl) => acl,
897-
Err(err) => {
898-
eprintln!("loading ACL \"{acl_file}\", {err}");
899-
return Err(crate::EXIT_CODE_LOAD_ACL_FAILURE.into());
900-
}
901-
};
886+
let acl = AccessControl::load_from_file(acl_file)
887+
.map_err(|err| ShadowsocksError::LoadAclFailure(format!("loading ACL \"{acl_file}\", {err}")))?;
902888
config.acl = Some(acl);
903889
}
904890

@@ -953,17 +939,15 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
953939
// DONE READING options
954940

955941
if config.local.is_empty() {
956-
eprintln!(
942+
return Err(ShadowsocksError::InsufficientParams(format!(
957943
"missing `local_address`, consider specifying it by --local-addr command line option, \
958944
or \"local_address\" and \"local_port\" in configuration file"
959-
);
960-
return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into());
945+
)));
961946
}
962947

963-
if let Err(err) = config.check_integrity() {
964-
eprintln!("config integrity check failed, {err}");
965-
return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
966-
}
948+
config
949+
.check_integrity()
950+
.map_err(|err| ShadowsocksError::LoadConfigFailure(format!("config integrity check failed, {err}")))?;
967951

968952
#[cfg(unix)]
969953
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
973957

974958
#[cfg(unix)]
975959
if let Some(uname) = matches.get_one::<String>("USER") {
976-
if let Err(err) = crate::sys::run_as_user(uname) {
977-
eprintln!("failed to change as user, error: {err}");
978-
return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into());
979-
}
960+
crate::sys::run_as_user(uname).map_err(|err| {
961+
ShadowsocksError::InsufficientParams(format!("failed to change as user, error: {err}"))
962+
})?;
980963
}
981964

982965
info!("shadowsocks local {} build {}", crate::VERSION, crate::BUILD_TIME);
@@ -1031,19 +1014,17 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
10311014
match server_res {
10321015
// Server future resolved without an error. This should never happen.
10331016
Ok(..) => {
1034-
eprintln!("server exited unexpectedly");
1035-
return crate::EXIT_CODE_SERVER_EXIT_UNEXPECTEDLY.into();
1017+
return Err(ShadowsocksError::ServerExitUnexpectedly("server exited unexpectedly".to_owned()));
10361018
}
10371019
// Server future resolved with error, which are listener errors in most cases
10381020
Err(err) => {
1039-
eprintln!("server aborted with {err}");
1040-
return crate::EXIT_CODE_SERVER_ABORTED.into();
1021+
return Err(ShadowsocksError::ServerAborted(format!("server aborted with {err}")));
10411022
}
10421023
}
10431024
}
10441025
// The abort signal future resolved. Means we should just exit.
10451026
_ = abort_signal => {
1046-
return ExitCode::SUCCESS;
1027+
return Ok(());
10471028
}
10481029
_ = reload_task => {
10491030
// continue.
@@ -1059,9 +1040,12 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
10591040
/// Program entrance `main`
10601041
#[inline]
10611042
pub fn main(matches: &ArgMatches) -> ExitCode {
1062-
match create(matches) {
1063-
Ok((runtime, main_fut)) => runtime.block_on(main_fut),
1064-
Err(code) => code,
1043+
match create(matches).and_then(|(runtime, main_fut)| runtime.block_on(main_fut)) {
1044+
Ok(()) => ExitCode::SUCCESS,
1045+
Err(err) => {
1046+
eprintln!("{err}");
1047+
err.exit_code().into()
1048+
}
10651049
}
10661050
}
10671051

‎src/service/manager.rs‎

Lines changed: 29 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ use shadowsocks_service::{
2727
use crate::logging;
2828
use crate::{
2929
config::{Config as ServiceConfig, RuntimeMode},
30+
error::{ShadowsocksError, ShadowsocksResult},
3031
monitor, vparser,
3132
};
3233

@@ -267,7 +268,7 @@ pub fn define_command_line_options(mut app: Command) -> Command {
267268
}
268269

269270
/// Create `Runtime` and `main` entry
270-
pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = ExitCode>), ExitCode> {
271+
pub fn create(matches: &ArgMatches) -> ShadowsocksResult<(Runtime, impl Future<Output = ShadowsocksResult>)> {
271272
let (config, runtime) = {
272273
let config_path_opt = matches.get_one::<PathBuf>("CONFIG").cloned().or_else(|| {
273274
if !matches.contains_id("SERVER_CONFIG") {
@@ -284,13 +285,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
284285
});
285286

286287
let mut service_config = match config_path_opt {
287-
Some(ref config_path) => match ServiceConfig::load_from_file(config_path) {
288-
Ok(c) => c,
289-
Err(err) => {
290-
eprintln!("loading config {config_path:?}, {err}");
291-
return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
292-
}
293-
},
288+
Some(ref config_path) => ServiceConfig::load_from_file(config_path)
289+
.map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {config_path:?}, {err}")))?,
294290
None => ServiceConfig::default(),
295291
};
296292
service_config.set_options(matches);
@@ -308,13 +304,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
308304
trace!("{:?}", service_config);
309305

310306
let mut config = match config_path_opt {
311-
Some(cpath) => match Config::load_from_file(&cpath, ConfigType::Manager) {
312-
Ok(cfg) => cfg,
313-
Err(err) => {
314-
eprintln!("loading config {cpath:?}, {err}");
315-
return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
316-
}
317-
},
307+
Some(cpath) => Config::load_from_file(&cpath, ConfigType::Manager)
308+
.map_err(|err| ShadowsocksError::LoadConfigFailure(format!("loading config {cpath:?}, {err}")))?,
318309
None => Config::new(ConfigType::Manager),
319310
};
320311

@@ -421,13 +412,8 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
421412
}
422413

423414
if let Some(acl_file) = matches.get_one::<String>("ACL") {
424-
let acl = match AccessControl::load_from_file(acl_file) {
425-
Ok(acl) => acl,
426-
Err(err) => {
427-
eprintln!("loading ACL \"{acl_file}\", {err}");
428-
return Err(crate::EXIT_CODE_LOAD_ACL_FAILURE.into());
429-
}
430-
};
415+
let acl = AccessControl::load_from_file(acl_file)
416+
.map_err(|err| ShadowsocksError::LoadAclFailure(format!("loading ACL \"{acl_file}\", {err}")))?;
431417
config.acl = Some(acl);
432418
}
433419

@@ -470,18 +456,16 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
470456

471457
// DONE reading options
472458

473-
if config.manager.is_none() {
474-
eprintln!(
459+
config.manager.as_ref().ok_or_else(|| {
460+
ShadowsocksError::InsufficientParams(format!(
475461
"missing `manager_address`, consider specifying it by --manager-address command line option, \
476462
or \"manager_address\" and \"manager_port\" keys in configuration file"
477-
);
478-
return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into());
479-
}
463+
))
464+
})?;
480465

481-
if let Err(err) = config.check_integrity() {
482-
eprintln!("config integrity check failed, {err}");
483-
return Err(crate::EXIT_CODE_LOAD_CONFIG_FAILURE.into());
484-
}
466+
config
467+
.check_integrity()
468+
.map_err(|err| ShadowsocksError::LoadConfigFailure(format!("config integrity check failed, {err}")))?;
485469

486470
#[cfg(unix)]
487471
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
491475

492476
#[cfg(unix)]
493477
if let Some(uname) = matches.get_one::<String>("USER") {
494-
if let Err(err) = crate::sys::run_as_user(uname) {
495-
eprintln!("failed to change as user, error: {err}");
496-
return Err(crate::EXIT_CODE_INSUFFICIENT_PARAMS.into());
497-
}
478+
crate::sys::run_as_user(uname).map_err(|err| {
479+
ShadowsocksError::InsufficientParams(format!("failed to change as user, error: {err}"))
480+
})?;
498481
}
499482

500483
info!("shadowsocks manager {} build {}", crate::VERSION, crate::BUILD_TIME);
@@ -526,17 +509,13 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
526509

527510
match future::select(server, abort_signal).await {
528511
// Server future resolved without an error. This should never happen.
529-
Either::Left((Ok(..), ..)) => {
530-
eprintln!("server exited unexpectedly");
531-
crate::EXIT_CODE_SERVER_EXIT_UNEXPECTEDLY.into()
532-
}
512+
Either::Left((Ok(..), ..)) => Err(ShadowsocksError::ServerExitUnexpectedly(
513+
"server exited unexpectedly".to_owned(),
514+
)),
533515
// Server future resolved with error, which are listener errors in most cases
534-
Either::Left((Err(err), ..)) => {
535-
eprintln!("server aborted with {err}");
536-
crate::EXIT_CODE_SERVER_ABORTED.into()
537-
}
516+
Either::Left((Err(err), ..)) => Err(ShadowsocksError::ServerAborted(format!("server aborted with {err}"))),
538517
// The abort signal future resolved. Means we should just exit.
539-
Either::Right(_) => ExitCode::SUCCESS,
518+
Either::Right(_) => Ok(()),
540519
}
541520
};
542521

@@ -546,9 +525,12 @@ pub fn create(matches: &ArgMatches) -> Result<(Runtime, impl Future<Output = Exi
546525
/// Program entrance `main`
547526
#[inline]
548527
pub fn main(matches: &ArgMatches) -> ExitCode {
549-
match create(matches) {
550-
Ok((runtime, main_fut)) => runtime.block_on(main_fut),
551-
Err(code) => code,
528+
match create(matches).and_then(|(runtime, main_fut)| runtime.block_on(main_fut)) {
529+
Ok(()) => ExitCode::SUCCESS,
530+
Err(err) => {
531+
eprintln!("{err}");
532+
err.exit_code().into()
533+
}
552534
}
553535
}
554536

0 commit comments

Comments
 (0)