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

Should Tesla.Adapter.Mint handle %Mint.TransportError? #376

Closed
tegon opened this issue Apr 15, 2020 · 1 comment
Closed

Should Tesla.Adapter.Mint handle %Mint.TransportError? #376

tegon opened this issue Apr 15, 2020 · 1 comment
Labels
bug 🔥 help wanted mint Issues related to mint adapter

Comments

@tegon
Copy link
Contributor

tegon commented Apr 15, 2020

Context

I have an app running in production with Tesla + Mint and I received the following error the other day:

(CaseClauseError) no case clause matching: {:error, %Mint.TransportError{reason: :timeout}}

This confused me a bit, since I already had a clause matching timeouts - {:error, :timeout} - which was working in my tests.
I had to dive into both Tesla and Mint's source until I found out the reason this happened. Tesla uses the timeout key passed to the Mint adapter as a way to halt when no messages are received from Mint in that given period. That's what returns the {:error, :timeout} tuple, and what I saw before doing manual tests.
Meanwhile, Mint also has a timeout option that can be set by passing transport_opts: [timeout: timeout]. I was able to reproduce the error from production by passing 0 as the timeout there. Something like the following:

middleware = [
  {Tesla.Middleware.BaseUrl, config[:base_url]},
  Tesla.Middleware.JSON,
  Tesla.Middleware.Telemetry
]

adapter = {Tesla.Adapter.Mint, timeout: config[:timeout], transport_opts: [timeout: 0]}

Tesla.client(middleware, adapter)

Which means that if Mint sends a message saying that the request timed out before Tesla's configured timeout, clients will receive the {:error, %Mint.TransportError{reason: :timeout}} back.

Next steps

I guess the thing that confused me the most is that I wasn't expecting that error to bubble up since the timeout was already configured and handled. I handled this new cause in my app for now but I thought it would be a good idea to bring this up to discussion to see what you all think of it. I have a couple of questions:

  • Should the Tesla Mint adapter handle these kinds of errors and return them in the {:error, term} format?
  • Is there a reason for Tesla not to use the transport_opts when calling Mint.HTTP.connect/4?

I think it might be a good idea to handle these errors inside the adapter so that the clients have a standard way of handling errors on their side, without getting too adapter-specific. If you think this is a good idea, I'm happy to work on it and send a PR back.

Otherwise, at least some documentation around this would be a valuable addition so that this doesn't surprise others too.

@teamon
Copy link
Member

teamon commented Apr 16, 2020

This is somehow related to #255

The tl;dr is - timeout is hard.

Every adapter does it a bit differently and there are different kinds of timeouts (i.e. hackney's recv_timeout is for each recv() call only, not the total request/response exchange).

I've started working on #255 but stopped as I haven't found a good solution.
If you have any ideas on how to solve this I'm all ears.

@teamon teamon added bug 🔥 help wanted mint Issues related to mint adapter labels Apr 16, 2020
@teamon teamon added this to the 1.4 milestone Apr 16, 2020
@teamon teamon modified the milestones: 1.4, 1.5 Nov 15, 2020
@tegon tegon closed this as completed Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🔥 help wanted mint Issues related to mint adapter
Projects
None yet
Development

No branches or pull requests

2 participants