From a16fb7c35c3b556c28b8e7ad708f6129b2aad161 Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Thu, 30 Apr 2020 15:07:21 +0300 Subject: clean up and test coverage --- lib/mix/tasks/pleroma/benchmark.ex | 6 --- lib/pleroma/gun/conn.ex | 9 ++-- lib/pleroma/http/middleware/follow_redirects.ex | 1 + lib/pleroma/pool/connections.ex | 22 ++++---- lib/pleroma/pool/supervisor.ex | 3 +- lib/pleroma/reverse_proxy/client/tesla.ex | 2 - test/http/gun_test.exs | 8 +++ test/pool/connections_test.exs | 67 ++++++++++++++++++++++++- 8 files changed, 87 insertions(+), 31 deletions(-) diff --git a/lib/mix/tasks/pleroma/benchmark.ex b/lib/mix/tasks/pleroma/benchmark.ex index 3d37dfa24..3a124a221 100644 --- a/lib/mix/tasks/pleroma/benchmark.ex +++ b/lib/mix/tasks/pleroma/benchmark.ex @@ -79,12 +79,6 @@ def run(["render_timeline", nickname | _] = args) do def run(["adapters"]) do start_pleroma() - :ok = - Pleroma.Gun.Conn.open( - "https://httpbin.org/stream-bytes/1500", - :gun_connections - ) - Process.sleep(1_500) Benchee.run( diff --git a/lib/pleroma/gun/conn.ex b/lib/pleroma/gun/conn.ex index de3173351..fb7153a16 100644 --- a/lib/pleroma/gun/conn.ex +++ b/lib/pleroma/gun/conn.ex @@ -34,11 +34,8 @@ defmodule Pleroma.Gun.Conn do crf: 1, retries: 0 - @spec open(String.t() | URI.t(), atom(), keyword()) :: :ok | nil - def open(url, name, opts \\ []) - def open(url, name, opts) when is_binary(url), do: open(URI.parse(url), name, opts) - - def open(%URI{} = uri, name, opts) do + @spec open(URI.t(), atom(), keyword()) :: :ok + def open(%URI{} = uri, name, opts \\ []) do pool_opts = Pleroma.Config.get([:connections_pool], []) opts = @@ -62,7 +59,7 @@ def open(%URI{} = uri, name, opts) do end defp try_open(name, uri, opts, max_connections) do - if Connections.count(name) < max_connections do + if Connections.count(name) <= max_connections do do_open(uri, opts) else close_least_used_and_do_open(name, uri, opts) diff --git a/lib/pleroma/http/middleware/follow_redirects.ex b/lib/pleroma/http/middleware/follow_redirects.ex index bed3ec9a0..c81755a48 100644 --- a/lib/pleroma/http/middleware/follow_redirects.ex +++ b/lib/pleroma/http/middleware/follow_redirects.ex @@ -1,4 +1,5 @@ # Pleroma: A lightweight social networking server +# Copyright © 2015-2020 Tymon Tobolski # Copyright © 2017-2020 Pleroma Authors # SPDX-License-Identifier: AGPL-3.0-only diff --git a/lib/pleroma/pool/connections.ex b/lib/pleroma/pool/connections.ex index 86bb00bc5..87fa50a22 100644 --- a/lib/pleroma/pool/connections.ex +++ b/lib/pleroma/pool/connections.ex @@ -16,21 +16,20 @@ defmodule Pleroma.Pool.Connections do @type seconds :: pos_integer() @type t :: %__MODULE__{ - conns: %{domain() => conn()}, - opts: keyword() + conns: %{domain() => conn()} } - defstruct conns: %{}, opts: [] + defstruct conns: %{} - @spec start_link({atom(), keyword()}) :: {:ok, pid()} - def start_link({name, opts}) do - GenServer.start_link(__MODULE__, opts, name: name) + @spec start_link(atom()) :: {:ok, pid()} + def start_link(name) do + GenServer.start_link(__MODULE__, [], name: name) end @impl true - def init(opts) do + def init(_) do schedule_close_idle_conns() - {:ok, %__MODULE__{conns: %{}, opts: opts}} + {:ok, %__MODULE__{conns: %{}}} end @spec checkin(String.t() | URI.t(), atom()) :: pid() | nil @@ -43,11 +42,8 @@ def checkin(%URI{} = uri, name, opts) do @spec alive?(atom()) :: boolean() def alive?(name) do - if pid = Process.whereis(name) do - Process.alive?(pid) - else - false - end + pid = Process.whereis(name) + is_pid(pid) and Process.alive?(pid) end @spec get_state(atom()) :: t() diff --git a/lib/pleroma/pool/supervisor.ex b/lib/pleroma/pool/supervisor.ex index faf646cb2..5e200259d 100644 --- a/lib/pleroma/pool/supervisor.ex +++ b/lib/pleroma/pool/supervisor.ex @@ -15,8 +15,7 @@ def start_link(args) do def init(_) do conns_child = %{ id: Pool.Connections, - start: - {Pool.Connections, :start_link, [{:gun_connections, Config.get([:connections_pool])}]} + start: {Pool.Connections, :start_link, [:gun_connections]} } Supervisor.init([conns_child | pools()], strategy: :one_for_one) diff --git a/lib/pleroma/reverse_proxy/client/tesla.ex b/lib/pleroma/reverse_proxy/client/tesla.ex index 999b1a4cb..50736dabb 100644 --- a/lib/pleroma/reverse_proxy/client/tesla.ex +++ b/lib/pleroma/reverse_proxy/client/tesla.ex @@ -33,8 +33,6 @@ def request(method, url, headers, body, opts \\ []) do else {:ok, response.status, response.headers} end - else - {:error, error} -> {:error, error} end end diff --git a/test/http/gun_test.exs b/test/http/gun_test.exs index c64cf7125..1a5a5b60c 100644 --- a/test/http/gun_test.exs +++ b/test/http/gun_test.exs @@ -9,6 +9,14 @@ defmodule Pleroma.HTTP.GunTest do alias Pleroma.HTTP.Gun describe "options/1" do + test "http" do + uri = URI.parse("http://example.com") + + opts = Gun.options([reuse_conn: false], uri) + + assert opts[:reuse_conn] == false + end + test "https url with default port" do uri = URI.parse("https://example.com") diff --git a/test/pool/connections_test.exs b/test/pool/connections_test.exs index d8436997f..8490f6bf1 100644 --- a/test/pool/connections_test.exs +++ b/test/pool/connections_test.exs @@ -26,7 +26,7 @@ defmodule Pleroma.Pool.ConnectionsTest do setup do name = :test_connections - {:ok, pid} = Connections.start_link({name, []}) + {:ok, pid} = Connections.start_link(name) on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(name) @@ -97,6 +97,66 @@ test "returns false if not started" do end end + describe "pool overflow" do + setup do: clear_config([:connections_pool, :max_connections], 2) + + test "when all conns are active return nil", %{name: name} do + open_mock(2) + conn1 = Connections.checkin("https://example1.com", name) + conn2 = Connections.checkin("https://example2.com", name) + refute Connections.checkin("https://example3.com", name) + + self = self() + + assert match?( + %Connections{ + conns: %{ + "https:example1.com:443" => %Conn{ + conn: ^conn1, + used_by: [{^self, _}] + }, + "https:example2.com:443" => %Conn{ + conn: ^conn2, + used_by: [{^self, _}] + } + } + }, + Connections.get_state(name) + ) + + assert Connections.count(name) == 2 + end + + test "close idle conn", %{name: name} do + open_mock(3) + |> expect(:close, fn _ -> :ok end) + + self = self() + conn1 = Connections.checkin("https://example1.com", name) + Connections.checkout(conn1, self, name) + conn2 = Connections.checkin("https://example2.com", name) + conn3 = Connections.checkin("https://example3.com", name) + + assert match?( + %Connections{ + conns: %{ + "https:example2.com:443" => %Conn{ + conn: ^conn2, + used_by: [{^self, _}] + }, + "https:example3.com:443" => %Conn{ + conn: ^conn3, + used_by: [{^self, _}] + } + } + }, + Connections.get_state(name) + ) + + assert Connections.count(name) == 2 + end + end + test "opens connection and reuse it on next request", %{name: name} do open_mock() url = "http://some-domain.com" @@ -376,6 +436,9 @@ test "connection can't get up", %{name: name} do refute Connections.checkin(url, name) end) =~ "Opening connection to http://gun-not-up.com failed with error {:error, :timeout}" + + state = Connections.get_state(name) + assert state.conns == %{} end test "process gun_down message and then gun_up", %{name: name} do @@ -784,7 +847,7 @@ test "lower crf and lower reference", %{name: name} do test "count/1" do name = :test_count - {:ok, _} = Connections.start_link({name, []}) + {:ok, _} = Connections.start_link(name) assert Connections.count(name) == 0 Connections.add_conn(name, "1", %Conn{conn: self()}) assert Connections.count(name) == 1 -- cgit v1.2.3