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

Use exit/1 instead of Process.exit/2 on batch timeout #1348

Conversation

martosaur
Copy link
Contributor

Hi 👋

#1298 replaced Task.await with a combination of Task.shutdown + Process.exit/2 in the case of timeout. This was theoretically an equivalent, as with Task.await, if the timeout is exceeded the caller process exits. However, Task.await uses Kernel.exit/1 under the hood which AFAIK isn't equivalent to Process.exit(self(), reason).

Here's a single file demo of errors:

Script
Mix.install([
  {:absinthe, "1.7.6"},
  {:absinthe_plug, "~> 1.5"},
  {:jason, "~> 1.4"},
  {:plug_cowboy, "~> 2.5"},
  {:bandit, "~> 1.1"}
])

defmodule TimeoutModule do
  def arbitrary_fn_name(_, _) do
    :timer.sleep(2000)
  end
end

defmodule Schema do
  use Absinthe.Schema

  query do
    field :timeout, :string do
      resolve(fn _, _, _ ->
        batch(
          {TimeoutModule, :arbitrary_fn_name, %{arbitrary: :data}},
          nil,
          fn batch -> {:ok, batch} end,
          timeout: 1
        )
      end)
    end
  end
end

defmodule Router do
  use Plug.Router
  plug(Plug.Logger)
  plug(:match)
  plug(:dispatch)

  plug(Plug.Parsers,
    parsers: [:urlencoded, :multipart, :json, Absinthe.Plug.Parser],
    pass: ["*/*"],
    json_decoder: Jason
  )

  forward("/api",
    to: Absinthe.Plug,
    init_opts: [schema: Schema]
  )
end

plug_cowboy = {Plug.Cowboy, plug: Router, scheme: :http, port: 4000}
bandit = {Bandit, plug: Router, scheme: :http, port: 4001}
require Logger
Logger.info("starting #{inspect(plug_cowboy)}")
{:ok, _} = Supervisor.start_link([plug_cowboy], strategy: :one_for_one)
{:ok, _} = Supervisor.start_link([bandit], strategy: :one_for_one)

# unless running from IEx, sleep idenfinitely so we can serve requests
unless IEx.started?() do
  Process.sleep(:infinity)
end

# Example request
# curl --request POST \
#   --url http://127.0.0.1:4000/api \
#   --data '{timeout}'

This is 1.7.6 error with cowboy (before #1298)

14:10:09.187 [error] #PID<0.3035.0> running Router (connection #PID<0.3034.0>, stream id 1) terminated
Server: 127.0.0.1:4000 (http)
Request: POST /api
** (exit) exited in: Task.await(%Task{mfa: {:erlang, :apply, 2}, owner: #PID<0.3035.0>, pid: #PID<0.3036.0>, ref: #Reference<0.0.388483.1747014105.723582980.237863>}, 1)
    ** (EXIT) time out

On current main cowboy just hangs, but Bandit yields this error which demonstrates that execution continues after Process.exit(self(), :timeout) is called:

14:16:14.920 [error] ** (Plug.Conn.WrapperError) ** (ArgumentError) argument error
    (stdlib 6.1.2) :maps.from_list([true])
    (elixir 1.17.3) lib/map.ex:263: Map.new_from_enum/2
    (absinthe 1.7.8) lib/absinthe/middleware/batch.ex:130: Absinthe.Middleware.Batch.after_resolution/1
    (elixir 1.17.3) lib/enum.ex:2531: Enum."-reduce/3-lists^foldl/2-0-"/3
    (absinthe 1.7.8) lib/absinthe/phase/document/execution/resolution.ex:72: Absinthe.Phase.Document.Execution.Resolution.perform_resolution/3
    (absinthe 1.7.8) lib/absinthe/phase/document/execution/resolution.ex:24: Absinthe.Phase.Document.Execution.Resolution.resolve_current/3
    (absinthe 1.7.8) lib/absinthe/pipeline.ex:408: Absinthe.Pipeline.run_phase/3
    (absinthe_plug 1.5.8) lib/absinthe/plug.ex:536: Absinthe.Plug.run_query/4

Finally, with this PR the error as seen through Cowboy will be this:

14:19:26.918 [error] #PID<0.3966.0> running Router (connection #PID<0.3965.0>, stream id 1) terminated
Server: 127.0.0.1:4000 (http)
Request: POST /api
** (exit) {:timeout, 1, {TimeoutModule, :arbitrary_fn_name, %{arbitrary: :data}}}

Bandit does not handle process exit very well, but that's not new and a conversation for another ticket I think.

Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you!

@benwilson512 benwilson512 merged commit 389111c into absinthe-graphql:main Dec 8, 2024
6 checks passed
@martosaur martosaur deleted the am/immediately_exit_on_batch_timeout branch December 8, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants