Skip to content
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.

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

Add error messages for interrupt #704

Merged

Conversation

jamesray1
Copy link
Contributor

No description provided.

@jamesray1 jamesray1 mentioned this pull request Nov 29, 2018
@tomaka
Copy link
Member

tomaka commented Nov 29, 2018

The error should be an enum if anything, not a &str.

@jamesray1
Copy link
Contributor Author

jamesray1 commented Nov 30, 2018

Fixed, all tests pass with cargo test --all.

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM. It would be good to add a test for the new errors too. See interrupting_an_established_connection_is_err where we simply check if the outcome is_err().

@ghost ghost assigned dvdplm Dec 4, 2018
@ghost ghost added the in progress label Dec 4, 2018
@dvdplm dvdplm removed their assignment Dec 4, 2018
core/src/nodes/collection.rs Outdated Show resolved Hide resolved
pub enum InterruptErr {
/// The task entry is vacant; it needs to be added first via add_reach_attempt
/// (with the TaskState set to Pending) before we try to connect.
VacantEntry,
Copy link
Contributor

Choose a reason for hiding this comment

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

While it is technically correct that the problem is the entry in the hashmap being vacant, the real error is that they're trying to interrupt using an invalid reach attempt. I think this error variant should reflect that. How about NoSuchReachAttemptId? or just ReachAttemptNotFound?

core/src/nodes/collection.rs Outdated Show resolved Hide resolved
core/src/nodes/collection.rs Outdated Show resolved Hide resolved
core/src/nodes/collection.rs Outdated Show resolved Hide resolved
core/src/nodes/collection.rs Outdated Show resolved Hide resolved
core/src/nodes/collection.rs Outdated Show resolved Hide resolved
@dvdplm
Copy link
Contributor

dvdplm commented Dec 5, 2018

@tomaka can you give this a once-over at your convenience?

@tomaka tomaka merged commit b2367e5 into libp2p:master Dec 5, 2018
@ghost ghost removed the in progress label Dec 5, 2018
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Dec 6, 2018
* upstream/master:
  Rename all the network behaviours to more basic names (libp2p#726)
  Avoid some warnings. (libp2p#733)
  Add error messages for interrupt (libp2p#704)
  Remove relay, peerstore and datastore (libp2p#723)
  Don't add an address to the topology if it is already in (libp2p#724)
  Add a few more methods to Swarm and PollParameters (libp2p#721)
  Some changes to the main libp2p doc (libp2p#710)
  Don't wrap yamux::Connection in a mutex (libp2p#719)
  relay: Use `SliceRandom::shuffle`. (libp2p#722)
  Remove some boxed futures. (libp2p#718)
  Fix several errors reported by clippy. (libp2p#715)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants