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

Timeout middleware behaviour doesn't make sense #130

Closed
edescourtis opened this issue Oct 24, 2017 · 5 comments
Closed

Timeout middleware behaviour doesn't make sense #130

edescourtis opened this issue Oct 24, 2017 · 5 comments
Assignees
Labels

Comments

@edescourtis
Copy link

I am using Tesla to reach many servers some of which I expect to be down. My Tesla related code is generated by swagger codegen. I was expecting from the timeout middleware to return some sort of error such as {:error, :unavailable} or {:error, :timeout} or something similar. Instead, the calling process gets a kill signal which makes it very annoying to work around (especially so when generated from swagger).

The Erlang philosophy is "let it crash" not "make it crash". The middleware needs to let the caller decide whether he should crash or not. Typically you want to handle expected cases (that includes expected errors) and to let it crash when something unexpected happens.

I will submit a PR later. How far back does Tesla support Elixir? Is supporting Elixir >= 1.3 enough?

@amatalai
Copy link
Collaborator

https://github.com/teamon/tesla/blob/master/lib/tesla/middleware/timeout.ex#L37
This kills task linked to current process. I'm not sure yet, why tests didn't detect such thing.

@edescourtis edescourtis changed the title Timeout middle behaviour doesn't make sense Timeout middleware behaviour doesn't make sense Oct 24, 2017
@edescourtis
Copy link
Author

PR is ready

@emerleite
Copy link
Contributor

@amatalai @edescourtis We need to stop somehow because the request will continue.

@emerleite
Copy link
Contributor

emerleite commented Nov 1, 2017

@edescourtis Your PR #131 makes sense and fixes it. We tested here and works fine.

@teamon teamon self-assigned this Nov 23, 2017
@teamon
Copy link
Member

teamon commented Nov 28, 2017

Fixed via #131

@teamon teamon closed this as completed Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants