Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Custom Error Messages on ENFILE and EMFILE IO Errors #8744

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions util/network/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@ ethereum-types = "0.3"
ethkey = { path = "../../ethkey" }
ipnetwork = "0.12.6"
rlp = { path = "../rlp" }
libc = "0.2.7"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to use 0.2 here – this code have quite unspecific version requirements.

snappy = { git = "https://github.com/paritytech/rust-snappy" }
59 changes: 58 additions & 1 deletion util/network/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

use std::{io, net, fmt};
use libc::{ENFILE, EMFILE};
use io::IoError;
use {rlp, ethkey, crypto, snappy};

Expand Down Expand Up @@ -83,7 +84,6 @@ impl fmt::Display for DisconnectReason {
error_chain! {
foreign_links {
SocketIo(IoError) #[doc = "Socket IO error."];
Io(io::Error) #[doc = "Error concerning the Rust standard library's IO subsystem."];
Decompression(snappy::InvalidInput) #[doc = "Decompression error."];
}

Expand Down Expand Up @@ -141,6 +141,34 @@ error_chain! {
description("Packet is too large"),
display("Packet is too large"),
}

#[doc = "We've reached system resource limits for this process"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I for one like it more impersonal: Reached process resource limits. :)

ProcessTooManyFiles {
description("Too many open files in process."),
display("Too many open files in this process. Check your resource limits and restart parity"),
}

#[doc = "We've reached system wide resource limits"]
SystemTooManyFiles {
description("Too many open files on system."),
display("Too many open files on system. Consider closing some processes/release some file handlers or increas the system-wide resource limits and restart parity."),
}

#[doc = "An unknown IO error occurred."]
Io(err: io::Error) {
description("IO Error"),
display("We've encoutered an IO Error: {:}", err),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: encountered

And I'd prefer Unexpected IO error: {}.

}
}
}

impl From<::std::io::Error> for Error {
fn from(err: ::std::io::Error) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::io namespace is already imported. be consistent ;)

match err.raw_os_error() {
Some(ENFILE) => ErrorKind::ProcessTooManyFiles.into(),
Some(EMFILE) => ErrorKind::SystemTooManyFiles.into(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach as it also easily allows us to add other more specific and better error messages for any OS-Error easily :) .

_ => Error::from_kind(ErrorKind::Io(err))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not super sure this is the super proper way to "chain" these errors within error-chain. However I was not able to figure out what else foreign_links is doing from the macro code base. I am happy for an error-chain export to give me a hint if there is a different way to do the general wrapping (without missing a backtrace - are we dropping a backtrace? I am unsure.)

}
}
}

Expand Down Expand Up @@ -191,3 +219,32 @@ fn test_errors() {
_ => panic!("Unexpected error"),
}
}

#[test]
fn test_io_errors() {
use libc::{EMFILE, ENFILE};

let en_file_error = io::Error::from_raw_os_error(ENFILE);

match *<Error as From<io::Error>>::from(en_file_error).kind() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use assert_matches here and below? This is a little hard to read imo.

ErrorKind::ProcessTooManyFiles => {},
_ => panic!("Unexpected error"),
}


let em_file_error = io::Error::from_raw_os_error(EMFILE);

match *<Error as From<io::Error>>::from(em_file_error).kind() {
ErrorKind::SystemTooManyFiles => {},
_ => panic!("Unexpected error"),
}


let usual_error = io::Error::from_raw_os_error(0);

match *<Error as From<io::Error>>::from(usual_error).kind() {
ErrorKind::Io(_) => {},
_ => panic!("Unexpected error"),
}

}
1 change: 1 addition & 0 deletions util/network/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ extern crate ethkey;
extern crate rlp;
extern crate ipnetwork;
extern crate snappy;
extern crate libc;

#[macro_use]
extern crate error_chain;
Expand Down