clean up and test coveragegun-memory-leak

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 @@ defmodule Mix.Tasks.Pleroma.Benchmark 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 @@ defmodule Pleroma.Gun.Conn 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 <https://github.com/teamon/tesla/blob/master/lib/tesla/middleware/follow_redirects.ex>
 # Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
 # 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 @@ defmodule Pleroma.Pool.Connections 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 @@ defmodule Pleroma.Pool.Supervisor 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 @@ defmodule Pleroma.ReverseProxy.Client.Tesla 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 @@ defmodule Pleroma.Pool.ConnectionsTest 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 @@ defmodule Pleroma.Pool.ConnectionsTest 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 @@ defmodule Pleroma.Pool.ConnectionsTest 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