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

RunError and few more error types implements Error #501

Merged
merged 1 commit into from
Jul 24, 2018
Merged

RunError and few more error types implements Error #501

merged 1 commit into from
Jul 24, 2018

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Jul 22, 2018

This allows them to be used with things like failure.

This is for #491.

The descriptions are a bit unhelpful I fear, but that's because I don't know what the error means and when it can actually happen. The doc's don't say and I gave up tracking it after few layers of things calling other things and translating errors around. Nevertheless, I think this is still a step forward.

fn description(&self) -> &str {
"Block error"
}
fn cause(&self) -> Option<&Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need to provide these cause functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no, I don't (I thought both were mandatory, but they are both provided in fact).

I'd still like to keep the one in the wrapper RunError ‒ as it delegates to the inner error, this is more robust for the future if the inner error might change the implementation.

@kpp
Copy link
Contributor

kpp commented Jul 22, 2018

Use special github syntax: Fixes #491 to automatically close #491 when this PR is merged

This allows them to be used with things like `failure`.
@vorner
Copy link
Contributor Author

vorner commented Jul 22, 2018

Um… I guess this change should not be able to create data races, right?

https://travis-ci.org/tokio-rs/tokio/jobs/406797195

self.inner.description()
}
fn cause(&self) -> Option<&Error> {
self.inner.cause()
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this should be self.inner.cause() and not just self.inner?

Copy link
Member

Choose a reason for hiding this comment

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

Using self.inner.cause() is the correct strategy here.

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 wasn't sure. But I felt this was actually the same error, just dressed in other clothes than one error causing another ‒ it wasn't like „Permission denied → Can't load configuration“ relation.

@carllerche
Copy link
Member

@vorner The tsan error is unrelated. It looks like it is caused by a fence in std. It looks like it is this item, but the type got moved in std.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

@carllerche carllerche merged commit 84db325 into tokio-rs:master Jul 24, 2018
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.

None yet

4 participants