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

Mint adapter and :unknown messages #357

Open
jlgeering opened this issue Feb 17, 2020 · 7 comments
Open

Mint adapter and :unknown messages #357

jlgeering opened this issue Feb 17, 2020 · 7 comments
Labels
bug 🔥 mint Issues related to mint adapter

Comments

@jlgeering
Copy link

according to Mint's documentation (see https://hexdocs.pm/mint/Mint.HTTP.html#stream/2) Mint.HTTP.stream/2 (called here: https://github.com/teamon/tesla/blob/v1.3.2/lib/tesla/adapter/mint.ex#L316) will return :unknown if the message passed to it is not destined for the current connection. The documentation suggests to handle this like a normal message:

receive do
  message ->
    case Mint.HTTP.stream(conn, message) do
      :unknown -> handle_normal_message(message)
      {:ok, conn, responses} -> handle_responses(conn, responses)
    end
end

long story short, this seems to indicate to me that messages that stream/2 labels as :unknown are not errors, just need to be handled differently (keep in mind: I am very much not an expert)

the Mint adapter on the other hand, converts these into errors (from https://github.com/teamon/tesla/blob/v1.3.2/lib/tesla/adapter/mint.ex#L307):

    defp receive_packet(conn, ref, opts, acc \\ %{}) do
      with {:ok, conn, responses} <- receive_message(conn, opts),
           acc <- reduce_responses(responses, ref, acc) do
        {:ok, conn, acc}
      else
        {:error, error} ->
          if opts[:close_conn], do: {:ok, _conn} = close(conn)
          {:error, error}

        {:error, _conn, error, _res} ->
          if opts[:close_conn], do: {:ok, _conn} = close(conn)
          {:error, "Encounter Mint error #{inspect(error)}"}

        :unknown ->
          if opts[:close_conn], do: {:ok, _conn} = close(conn)
          {:error, :unknown}
      end
    end

    defp receive_message(conn, %{mode: :active} = opts) do
      receive do
        message ->
          HTTP.stream(conn, message)
      after
        opts[:timeout] -> {:error, :timeout}
      end
    end

We do observer this behaviour from time to time (we get {:error, :unknown} responses), but I don't know how to reproduce it / write a failing test for it.

@alex-strizhakov
Copy link
Contributor

Last time i was getting this type of errors, these were messages from another process (as far as i remember messages like {:plug_conn, :sent}).

Error can be reproduced by sending any message to process, which is holding mint connection, while it's waiting for response messages.

Maybe for this type of errors would be more suitable to add logging, ignore this message and recall receive_packet function again.

@teamon teamon added the mint Issues related to mint adapter label Mar 3, 2020
@teamon
Copy link
Member

teamon commented Mar 3, 2020

@ericmj Could you give us a hand here?

@teamon teamon added this to To do in v2.0 Mar 3, 2020
@ericmj
Copy link

ericmj commented Mar 3, 2020

I have opened an issue for a feature so that you can match on specific mint connection messages: elixir-mint/mint#247.

IIRC tesla opens a single connection, sends a request, and closes the connection. If that's correct you can run in passive mode instead so you don't have to manage any messages: https://hexdocs.pm/mint/Mint.HTTP.html#module-mode.

@teamon teamon removed this from To do in v2.0 Apr 7, 2020
@teamon teamon added this to the 1.4 milestone Apr 7, 2020
@xinz
Copy link
Contributor

xinz commented Apr 8, 2020

Hello, what's the next for this issue of Mint adapter? Please correct me if any missing.

1, Currently, Mint adapter uses :active mode with one-off connection by default, do we need to change use :passive mode?
2, Do we still need to support :conn option to reuse the connection from outside, in this use case, looks like Mint adapter needs to take more boundary conditions, what do you think about it?

Thanks!

@teamon
Copy link
Member

teamon commented Apr 16, 2020

I think the next step would be to use :passive mode indeed.
The later one would be to implement active mode with real connection pooling

Also related to #301

@teamon teamon modified the milestones: 1.4, 1.5 Nov 15, 2020
@teamon
Copy link
Member

teamon commented Sep 10, 2021

I think we should move on with the passive mode for mint adapter, and direct people to use finch for persistent connections & pooling.

Any thoughts on that?

@ericmj
Copy link

ericmj commented Sep 10, 2021

@teamon I think that's the right approach.

@teamon teamon added bug 🔥 and removed blocked labels May 8, 2022
@teamon teamon removed this from the 1.5 milestone May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🔥 mint Issues related to mint adapter
Projects
Status: Todo
Development

No branches or pull requests

5 participants