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

Convert lapin-async's Error type to failure #147

Merged
merged 2 commits into from
Nov 23, 2018

Conversation

kureuil
Copy link
Contributor

@kureuil kureuil commented Nov 23, 2018

Uses the same hack as lapin-futures' ErrorKind type to prevent exhaustive matching against the different kinds of errors.

Incidentally, I also had to increase the type length limit to 16 millions characters to have the tests pass on Travis, the code was unable to compile otherwise which is pretty strange.

Uses the same hack as lapin-futures' ErrorKind type to prevent
exhaustive matching against the different kinds of errors.
Copy link
Collaborator

@Keruspe Keruspe left a comment

Choose a reason for hiding this comment

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

Maybe we should add some note wrt the type_length_limit in the README and somewhere in the doc?

Otherwise that’s great, thanks!

@kureuil kureuil force-pushed the lapin-async-failure branch from bbc9f05 to 1d3eabe Compare November 23, 2018 15:02
@kureuil
Copy link
Contributor Author

kureuil commented Nov 23, 2018

I re-investigated the problem today, and removed 3 out of 5 occurences of type_length_limit. The 16 millions type limit is only needed in the examples/client.rs file, I suppose the nested futures are the real culprits for such a big value. I left a small comment on the remaining occurences hinting at the possible cause.

I don't think it's necessary to add a note to the README because this seems to be a relatively common problem when dealing with nested futures, and the compiler explicitely tells you to add this attribute to your crate when the problem arise, so the fix is easier than it might seem.

EDIT: It also doesn't occur on all platforms, I was unable to reproduce it on Windows, while I can consistently hit it when working on Linux. I don't know how it reacts on macOS, but it's probably more similar to Linux than Windows ?

I suppose this means we're writing Good Code (tm)
@kureuil kureuil force-pushed the lapin-async-failure branch from 1d3eabe to 67f929a Compare November 23, 2018 15:22
@Keruspe Keruspe merged commit bb07d29 into amqp-rs:master Nov 23, 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.

2 participants