-
Notifications
You must be signed in to change notification settings - Fork 350
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
Comments
The new interface for request function will be |
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
Then I can do:
|
@mroach Currently the easiest option would be to use 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). |
@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? |
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 So my question is, is there any other easier way? |
[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 breaksTesla.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:
:timeout
option to adapter spec:timeout
in every adapter:timeout
option instead ofTesla.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 asTesla.Middleware.Timeout
.recv_timeout
applies to singlerecv
call, while `Tesla.Middleware.Timeout wraps the whole request/response exchange.The text was updated successfully, but these errors were encountered: