Skip to content

fix(swarm): display cause of denied listen error #4232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jul 24, 2023
Merged

fix(swarm): display cause of denied listen error #4232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8ce9d2e
display cause of denied listen error
divagant-martian Jul 24, 2023
973e4d1
attempt at changelog
divagant-martian Jul 24, 2023
95095a6
swarm version up
divagant-martian Jul 24, 2023
25f4931
Update swarm/CHANGELOG.md
divagant-martian Jul 24, 2023
34c5f36
Merge branch 'master' into master
divagant-martian Jul 24, 2023
b607c67
Merge branch 'master' into master
divagant-martian Jul 24, 2023
48c2aa1
lockfile
divagant-martian Jul 24, 2023
5ffb4d0
Merge branch 'master' into master
divagant-martian Jul 24, 2023
ebc0a5f
workspace dep
divagant-martian Jul 24, 2023
bd820a9
Merge branch 'master' into master
mergify[bot] Jul 24, 2023
0080238
Merge branch 'master' into master
mergify[bot] Jul 24, 2023
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
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ libp2p-quic = { version = "0.8.0-alpha", path = "transports/quic" }
libp2p-relay = { version = "0.16.1", path = "protocols/relay" }
libp2p-rendezvous = { version = "0.13.0", path = "protocols/rendezvous" }
libp2p-request-response = { version = "0.25.1", path = "protocols/request-response" }
libp2p-swarm = { version = "0.43.1", path = "swarm" }
libp2p-swarm = { version = "0.43.2", path = "swarm" }
libp2p-swarm-derive = { version = "0.33.0", path = "swarm-derive" }
libp2p-swarm-test = { version = "0.2.0", path = "swarm-test" }
libp2p-tcp = { version = "0.40.0", path = "transports/tcp" }
Expand Down
6 changes: 6 additions & 0 deletions swarm/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 0.43.2 - unreleased
- Display the cause of a `ListenError::Denied`.
See [PR 4232]

[PR 4232]: https://github.com/libp2p/rust-libp2p/pull/4158

## 0.43.1

- Do not announce external address candidate before address translation, unless translation does not apply.
Expand Down
2 changes: 1 addition & 1 deletion swarm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ name = "libp2p-swarm"
edition = "2021"
rust-version = { workspace = true }
description = "The libp2p swarm"
version = "0.43.1"
version = "0.43.2"
authors = ["Parity Technologies <[email protected]>"]
license = "MIT"
repository = "https://github.com/libp2p/rust-libp2p"
Expand Down
4 changes: 2 additions & 2 deletions swarm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1684,8 +1684,8 @@ impl fmt::Display for ListenError {
ListenError::Transport(_) => {
write!(f, "Listen error: Failed to negotiate transport protocol(s)")
}
ListenError::Denied { .. } => {
write!(f, "Listen error")
ListenError::Denied { cause } => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This cause is already available as part of calling Error::source. Including it in the fmt::Display as well is kind of an anti-pattern because it leads to double-printing of errors.

If we print it here, we should remove it from Error::source. Also, if you use something like anyhow, the source would be printed which I personally find the better approach than printing it here because stringifies as "late" as possible.

Copy link
Contributor Author

@divagant-martian divagant-martian Jul 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

first let me explain the reason for this: we get an error, we print it and get no information at all about this. The implementation of Display comes before the one for Error (since Error: Display ) so it should be up to the Error implementation to deal with this differently if necessary.

If trait A depends on trait B where B on itself is useful and publicly available, I would argue it's an anti-pattern to decide B's implementation just to satisfy something about A. More so of if that comes down to removing important information

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be up to the Error implementation to deal with this differently if necessary.

The issue with that is that it is quite hard to go from a string implementation back to structured data.

This is btw also the "official" recommendation, see rust-lang/project-error-handling#27 (comment).

What is missing the Rust ecosystem is a default way of printing errors. Yes, errors can be displayed but if I understand the error handling project group correctly, then at some point, there will be a std-built-in way of printing an error that will correctly chain them together. Until then, users are encouraged to use things like anyhow or eyre.

Personally, I mildly prefer to keep the display impls short and instead return sources from Error::source. This is still correct from an API point of view: This particular error itself says what happened, i.e. it explains one layer of the entire stack. But looking at errors out of context is seldomly meaningful which is why one should go down the chain of sources.

If trait A depends on trait B where B on itself is useful and publicly available, I would argue it's an anti-pattern to decide B's implementation just to satisfy something about A. More so of if that comes down to removing important information

I'd say it is exactly the opposite actually. By stringifying the error + its sources in the Display impl, the Display impl (which is more general than the Error trait), makes an assumption what it is going to be used for (printing chains of errors) when technically, it shouldn't know about the concept of a "source error".

I am not too fussed which way we go until there is a definite answer from the Rust project but what I want to avoid is double printing of errors when used with a library like anyhow. To avoid this, the inner error needs to be removed from source. Are you open to sending a patch for this?

write!(f, "Listen error: Denied: {cause}")
}
ListenError::LocalPeerId { endpoint } => {
write!(f, "Listen error: Local peer ID at {endpoint:?}.")
Expand Down