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 :timeout option to every adapter #255

Open
3 tasks
teamon opened this issue Oct 25, 2018 · 6 comments
Open
3 tasks

Add :timeout option to every adapter #255

teamon opened this issue Oct 25, 2018 · 6 comments

Comments

@teamon
Copy link
Member

teamon commented Oct 25, 2018

[copy from #240]

Timeout middleware has been causing issues for a long long time (#151, #157, #237 and more).
The two biggest problems with it are the use of Task which breaks Tesla.Mock and overriding adapter-specific timeouts.

With a well-defined adapter interface we can introduce a common :timeout option for adapter and promote it instead of the middleware. If adapter's underlying http library supports it (like hackney) it will be same as setting some option (recv_timeout: opts[:timeout]), if the library does not implement it it will have to be implemented on the adapter level (most probably in a similar fashion as current timeout middleware).

The timeout middleware should probably stay as it is, since it provides a different semantics when used with Retry middleware - the timeout set would capture the whole operation of multiple requests while adapter-level timeout only works for a single request.

To-do list:

  • Add :timeout option to adapter spec
  • Implement :timeout in every adapter
  • Update documentation to promote :timeout option instead of Tesla.Middleware.Timeout

The issue turned out to be more complex than I first thought. For example, hackney has recv_timeout option, but it is not the same as Tesla.Middleware.Timeout. recv_timeout applies to single recv call, while `Tesla.Middleware.Timeout wraps the whole request/response exchange.

@RyanSiu1995
Copy link
Contributor

The new interface for request function will be request(method, url, headers, body, timeout, opts)
My plan is to use Task.await/1 to handle the timeout. If the user specify adapter specific timeout opt, like recv_timeout in hackney, this :timeout option will be overrided and ignored.
Does it sound reasonable for you?

@mroach
Copy link

mroach commented Apr 30, 2019

I'm not sure if this is the place to ask, but what do you think about being able to set the timeout on a per-request basis? For example if you have requests that must reply quickly or fail, and endpoints that are allowed to take a longer time, currently the only option would be separate modules for each endpoint. My hacky patch to try it out was simply adding this in call/3 in the middleware:

timeout = Keyword.get(env.opts, :timeout) || Keyword.get(opts, :timeout) || @default_timeout

Then I can do:

get("http://server.tld/rest/must_be_fast", opts: [timeout: 20])

@teamon
Copy link
Member Author

teamon commented May 6, 2019

@mroach Currently the easiest option would be to use Tesla.client/2 function to create a custom client for cases when you need something specific (and you can use that with any middleware).

Having said that, it makes sense to have per-request middleware options customization, but it would need to work consistently for all middlewares (i.e., no special case for timeout).

@mroach
Copy link

mroach commented Aug 1, 2019

@teamon Just to be sure I understand, the idea would be something like specifying options in a way that doesn't conflict between middleware and is consistent for middleware to access?

Something like:

get("http://server/api/data", opts: [Tesla.Middleware.Timeout: [timeout: 100]])

A bit verbose, but something along those lines to namespace the configuration and make it easy for middleware to detect?

@bmarkons
Copy link

Hi, I found this issue while I was trying to figure out how to provide timeout for single Tesla request. I managed to do it by modifying the client pre field and injecting Tesla.Middleware.Timeout into it. However, I would expect to provide timeout through option like @mroach suggested but it seems it is not possible yet.

So my question is, is there any other easier way?

@yordis
Copy link
Member

yordis commented Feb 16, 2023

@teamon would the ideal scenario here be to reserve a key in options or something that we would use in the adapters?

Related to #151 also

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Idea
Development

No branches or pull requests

5 participants