Merge remote-tracking branch 'origin/develop' into matrixmatrix

1 files changed, 53 insertions, 18 deletions

diff --git a/lib/pleroma/web/rich_media/parser.ex b/lib/pleroma/web/rich_media/parser.ex
index 5727fda18..d6b54943b 100644
--- a/lib/pleroma/web/rich_media/parser.ex
+++ b/lib/pleroma/web/rich_media/parser.ex
@@ -1,10 +1,12 @@
 # Pleroma: A lightweight social networking server
-# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
+# Copyright © 2017-2021 Pleroma Authors <https://pleroma.social/>
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Web.RichMedia.Parser do
   require Logger
 
+  @cachex Pleroma.Config.get([:cachex, :provider], Cachex)
+
   defp parsers do
     Pleroma.Config.get([:rich_media, :parsers])
   end
@@ -20,28 +22,61 @@ defmodule Pleroma.Web.RichMedia.Parser do
       with {:ok, data} <- get_cached_or_parse(url),
            {:ok, _} <- set_ttl_based_on_image(data, url) do
         {:ok, data}
-      else
-        {:error, {:invalid_metadata, data}} = e ->
-          Logger.debug(fn -> "Incomplete or invalid metadata for #{url}: #{inspect(data)}" end)
-          e
-
-        error ->
-          Logger.error(fn -> "Rich media error for #{url}: #{inspect(error)}" end)
-          error
       end
     end
 
     defp get_cached_or_parse(url) do
-      case Cachex.fetch!(:rich_media_cache, url, fn _ -> {:commit, parse_url(url)} end) do
-        {:ok, _data} = res ->
-          res
-
-        {:error, _} = e ->
-          ttl = Pleroma.Config.get([:rich_media, :failure_backoff], 60_000)
-          Cachex.expire(:rich_media_cache, url, ttl)
-          e
+      case @cachex.fetch(:rich_media_cache, url, fn ->
+             case parse_url(url) do
+               {:ok, _} = res ->
+                 {:commit, res}
+
+               {:error, reason} = e ->
+                 # Unfortunately we have to log errors here, instead of doing that
+                 # along with ttl setting at the bottom. Otherwise we can get log spam
+                 # if more than one process was waiting for the rich media card
+                 # while it was generated. Ideally we would set ttl here as well,
+                 # so we don't override it number_of_waiters_on_generation
+                 # times, but one, obviously, can't set ttl for not-yet-created entry
+                 # and Cachex doesn't support returning ttl from the fetch callback.
+                 log_error(url, reason)
+                 {:commit, e}
+             end
+           end) do
+        {action, res} when action in [:commit, :ok] ->
+          case res do
+            {:ok, _data} = res ->
+              res
+
+            {:error, reason} = e ->
+              if action == :commit, do: set_error_ttl(url, reason)
+              e
+          end
+
+        {:error, e} ->
+          {:error, {:cachex_error, e}}
       end
     end
+
+    defp set_error_ttl(_url, :body_too_large), do: :ok
+    defp set_error_ttl(_url, {:content_type, _}), do: :ok
+
+    # The TTL is not set for the errors above, since they are unlikely to change
+    # with time
+
+    defp set_error_ttl(url, _reason) do
+      ttl = Pleroma.Config.get([:rich_media, :failure_backoff], 60_000)
+      @cachex.expire(:rich_media_cache, url, ttl)
+      :ok
+    end
+
+    defp log_error(url, {:invalid_metadata, data}) do
+      Logger.debug(fn -> "Incomplete or invalid metadata for #{url}: #{inspect(data)}" end)
+    end
+
+    defp log_error(url, reason) do
+      Logger.warn(fn -> "Rich media error for #{url}: #{inspect(reason)}" end)
+    end
   end
 
   @doc """
@@ -73,7 +108,7 @@ defmodule Pleroma.Web.RichMedia.Parser do
       {:ok, ttl} when is_number(ttl) ->
         ttl = ttl * 1000
 
-        case Cachex.expire_at(:rich_media_cache, url, ttl) do
+        case @cachex.expire_at(:rich_media_cache, url, ttl) do
           {:ok, true} -> {:ok, ttl}
           {:ok, false} -> {:error, :no_key}
         end