diff --git a/lib/philomena/application.ex b/lib/philomena/application.ex index e2b95c52f..5c8bb1bbf 100644 --- a/lib/philomena/application.ex +++ b/lib/philomena/application.ex @@ -24,6 +24,9 @@ defmodule Philomena.Application do # Mailer {Task.Supervisor, name: Philomena.AsyncEmailSupervisor}, + # Image upload processing + {Task.Supervisor, name: Philomena.ImageUploadSupervisor}, + # Starts a worker by calling: Philomena.Worker.start_link(arg) # {Philomena.Worker, arg}, {Redix, name: :redix, host: Application.get_env(:philomena, :redis_host)}, diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index 202267f9d..856919529 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -401,8 +401,8 @@ defmodule Philomena.Images do defp sources_for_edit(sources), do: sources defp async_upload(image, upload) do - linked_pid = - spawn(fn -> + {:ok, upload_pid} = + Task.Supervisor.start_child(Philomena.ImageUploadSupervisor, fn -> # Make sure task will finish before VM exit Process.flag(:trap_exit, true) @@ -416,12 +416,12 @@ defmodule Philomena.Images do end) # Give the upload to the linked process - Plug.Upload.give_away(upload.path, linked_pid, self()) + Plug.Upload.give_away(upload.path, upload_pid, self()) # Free up the linked process - send(linked_pid, :ready) + send(upload_pid, :ready) - linked_pid + :ok end defp try_upload(image, retry_count) when retry_count < 100 do @@ -1477,14 +1477,14 @@ defmodule Philomena.Images do ## Examples iex> create_image(actor, %{"tag_input" => "safe"}, upload) - {:ok, %{image: %Image{}, upload_pid: pid}} + {:ok, %Image{}} iex> create_image(banned_actor, params, upload) {:error, :ban} """ @spec create_image(Actor.t(), map() | nil, PhilomenaMedia.Upload.t() | nil) :: - {:ok, image_upload()} + {:ok, Image.t()} | {:error, :ban | :unauthorized | :rate_limited | Ecto.Changeset.t()} def create_image(%Actor{user: user} = actor, params, upload) do with :ok <- verify_write_access(actor), @@ -1536,17 +1536,13 @@ defmodule Philomena.Images do |> Multi.transact_with_automatic_retry() |> case do {:ok, %{image: %Image{} = image}} -> - upload_pid = async_upload(image, upload) + :ok = async_upload(image, upload) image = Repo.preload(image, tags: :aliases) broadcast_image_create(image) - # Return the upload PID along with the created image so that the caller - # can control the lifecycle of the upload if needed. It's useful, for - # example for the seeding process to know when to delete the temp file - # used for uploading. - {:ok, %{image: image, upload_pid: upload_pid}} + {:ok, image} {:error, :action_reservation, :rate_limited, _changes} -> {:error, :rate_limited} @@ -1560,16 +1556,6 @@ defmodule Philomena.Images do end end - @typedoc """ - Result of the `upload_image/3` function. The image was created in the DB but an - upload process could still be running in the background with its PID given in the - `upload_pid` field. - """ - @type image_upload :: %{ - image: %Image{}, - upload_pid: pid - } - @doc group: "Moderation and lifecycle" @doc """ Returns the paginated approval queue for `actor`: unapproved images, oldest diff --git a/lib/philomena_web/controllers/api/json/image_controller.ex b/lib/philomena_web/controllers/api/json/image_controller.ex index e508ebada..ca9ced888 100644 --- a/lib/philomena_web/controllers/api/json/image_controller.ex +++ b/lib/philomena_web/controllers/api/json/image_controller.ex @@ -27,7 +27,7 @@ defmodule PhilomenaWeb.Api.Json.ImageController do upload = PhilomenaMedia.Upload.cast(image_params, "image") case Images.create_image(conn.assigns.actor, image_params, upload) do - {:ok, %{image: image}} -> + {:ok, image} -> render(conn, "show.json", image: image, interactions: []) {:error, %Ecto.Changeset{} = changeset} -> diff --git a/lib/philomena_web/controllers/image_controller.ex b/lib/philomena_web/controllers/image_controller.ex index ea01ff9c8..c27c556aa 100644 --- a/lib/philomena_web/controllers/image_controller.ex +++ b/lib/philomena_web/controllers/image_controller.ex @@ -92,7 +92,7 @@ defmodule PhilomenaWeb.ImageController do upload = PhilomenaMedia.Upload.cast(params["image"], "image") case Images.create_image(conn.assigns.actor, params["image"], upload) do - {:ok, %{image: image}} -> + {:ok, image} -> conn |> put_flash(:info, "Image created successfully.") |> redirect(to: ~p"/images/#{image}") diff --git a/priv/repo/seeds_development.exs b/priv/repo/seeds_development.exs index 1a0887623..8f85acc9f 100644 --- a/priv/repo/seeds_development.exs +++ b/priv/repo/seeds_development.exs @@ -84,7 +84,7 @@ for image_def <- resources["remote_images"] do upload ) |> case do - {:ok, %{image: image}} -> + {:ok, image} -> Images.create_image_approve(admin_actor, image.id) IO.puts("Created image ##{image.id}") diff --git a/test/philomena/comments_test.exs b/test/philomena/comments_test.exs index 69aca0b65..915fdaee8 100644 --- a/test/philomena/comments_test.exs +++ b/test/philomena/comments_test.exs @@ -1019,7 +1019,9 @@ defmodule Philomena.CommentsTest do test "a valid anonymous fingerprinted actor creates a comment with no author", %{image: image} do assert {:ok, %Comment{} = comment} = - Comments.create_comment(actor(nil), image.id, %{"body" => "An anonymous comment"}) + Comments.create_comment(actor(nil, ip: random_ip()), image.id, %{ + "body" => "An anonymous comment" + }) assert comment.user_id == nil assert comment.body == "An anonymous comment" diff --git a/test/philomena/images_test.exs b/test/philomena/images_test.exs index 09013a06b..98c654bbf 100644 --- a/test/philomena/images_test.exs +++ b/test/philomena/images_test.exs @@ -1,9 +1,11 @@ defmodule Philomena.ImagesTest do - use Philomena.DataCase, async: true + # async: false - successful image uploads spawn an upload process + # (Images.async_upload/2) that hits the Repo; it is only allowed on the + # sandbox connection in shared mode, which is enabled for sync tests. + use Philomena.DataCase, async: false import Ecto.Query - alias Ecto.Adapters.SQL.Sandbox alias Philomena.Events alias Philomena.Multi alias Philomena.ImageFaves @@ -30,6 +32,7 @@ defmodule Philomena.ImagesTest do alias PhilomenaQuery.Search alias PhilomenaQuery.SearchHelpers + import Philomena.AsyncUpload import Philomena.GalleriesFixtures import Philomena.FiltersFixtures import Philomena.ImagesFixtures @@ -4357,26 +4360,6 @@ defmodule Philomena.ImagesTest do |> DateTime.truncate(:second) end - # Waits for the background upload process a successful upload spawns to exit. - # It shares the test process's sandbox connection, so letting it outlive the - # test leaves it retrying against a dead owner. The spawned process is our - # direct child. - defp await_async_upload do - test_pid = self() - - for pid <- Process.list(), Process.info(pid, :parent) == {:parent, test_pid} do - ref = Process.monitor(pid) - - receive do - {:DOWN, ^ref, :process, ^pid, _reason} -> :ok - after - 5_000 -> raise "async upload process #{inspect(pid)} did not exit" - end - end - - :ok - end - describe "show_image/2" do test "an anonymous viewer loads a visible image with zero change counts" do image = image_fixture() @@ -4694,22 +4677,21 @@ defmodule Philomena.ImagesTest do end describe "create_image/3" do + setup do + allow_async_uploads() + end + test "a normal actor uploads an image and the row exists" do actor = actor(confirmed_user_fixture()) :ok = Events.subscribe_events() - assert {:ok, %{image: %Image{} = image, upload_pid: pid}} = + assert {:ok, %Image{} = image} = Images.create_image( actor, %{"tag_input" => "safe, solo, pony"}, media_png_upload() ) - # The background upload process finishes the persist/repair work against - # the Repo; in an async case it owns no sandbox connection, so grant it the - # test's before awaiting its exit. - Sandbox.allow(Repo, self(), pid) - assert Repo.get(Image, image.id) assert source_urls(image) == [] @@ -4722,7 +4704,7 @@ defmodule Philomena.ImagesTest do actor = actor(confirmed_user_fixture()) sources = ["https://example.com/first", "https://example.com/second"] - assert {:ok, %{image: image, upload_pid: pid}} = + assert {:ok, image} = Images.create_image( actor, %{ @@ -4737,7 +4719,6 @@ defmodule Philomena.ImagesTest do media_png_upload() ) - Sandbox.allow(Repo, self(), pid) assert source_urls(image) == Enum.sort(sources) await_async_upload() end @@ -4745,7 +4726,7 @@ defmodule Philomena.ImagesTest do test "ignores blank source rows during upload" do actor = actor(confirmed_user_fixture()) - assert {:ok, %{image: image, upload_pid: pid}} = + assert {:ok, image} = Images.create_image( actor, %{ @@ -4759,7 +4740,6 @@ defmodule Philomena.ImagesTest do media_png_upload() ) - Sandbox.allow(Repo, self(), pid) assert source_urls(image) == ["https://example.com/source"] await_async_upload() end @@ -4790,7 +4770,7 @@ defmodule Philomena.ImagesTest do |> Ecto.Changeset.change(images_count: 4) |> Repo.update!() - assert {:ok, %{image: image, upload_pid: pid}} = + assert {:ok, image} = Images.create_image( actor(user), %{"tag_input" => "safe, solo, pony"}, @@ -4805,7 +4785,6 @@ defmodule Philomena.ImagesTest do assert user_id == user.id - Sandbox.allow(Repo, self(), pid) await_async_upload() end @@ -4847,7 +4826,7 @@ defmodule Philomena.ImagesTest do actor = actor(confirmed_user_fixture()) track_rate_limit(actor, :image_create) - assert {:ok, %{image: %Image{}, upload_pid: pid}} = + assert {:ok, %Image{}} = Images.create_image( actor, %{"tag_input" => "safe, solo, pony"}, @@ -4857,9 +4836,6 @@ defmodule Philomena.ImagesTest do # Recording happens synchronously once create_image succeeds. assert rate_limit_count(actor, :image_create) == "1" - # Let the background upload process finish against the test's sandbox - # connection before the test exits. - Sandbox.allow(Repo, self(), pid) await_async_upload() end diff --git a/test/philomena/posts_test.exs b/test/philomena/posts_test.exs index e2728b3fe..e1cbd055e 100644 --- a/test/philomena/posts_test.exs +++ b/test/philomena/posts_test.exs @@ -826,7 +826,7 @@ defmodule Philomena.PostsTest do # and reaches the public forum/topic create; the engine records the post # with a nil user (anonymous attribution). assert {:ok, %Post{} = post} = - Posts.create_post(actor(nil), forum.short_name, topic.slug, %{ + Posts.create_post(actor(nil, ip: random_ip()), forum.short_name, topic.slug, %{ "body" => "An anonymous reply" }) diff --git a/test/philomena_web/controllers/api/json/image_controller_test.exs b/test/philomena_web/controllers/api/json/image_controller_test.exs index 53c899c79..132e3003e 100644 --- a/test/philomena_web/controllers/api/json/image_controller_test.exs +++ b/test/philomena_web/controllers/api/json/image_controller_test.exs @@ -4,6 +4,7 @@ defmodule PhilomenaWeb.Api.Json.ImageControllerTest do # sandbox connection in shared mode, which ConnCase enables for sync tests. use PhilomenaWeb.ConnCase, async: false + import Philomena.AsyncUpload import Philomena.ImagesFixtures import Philomena.UsersFixtures @@ -14,6 +15,10 @@ defmodule PhilomenaWeb.Api.Json.ImageControllerTest do @png_fixture Path.absname("test/support/fixtures/files/upload-test.png") + setup_all do + allow_async_uploads() + end + describe "GET /api/v1/json/images/:id" do test "shows an image with the full representation set", %{conn: conn} do image = image_fixture(sources: ["https://example.com/art/1", "https://example.com/art/2"]) diff --git a/test/philomena_web/controllers/image_controller_test.exs b/test/philomena_web/controllers/image_controller_test.exs index efc534ad0..4025cdb76 100644 --- a/test/philomena_web/controllers/image_controller_test.exs +++ b/test/philomena_web/controllers/image_controller_test.exs @@ -3,6 +3,7 @@ defmodule PhilomenaWeb.ImageControllerTest do @moduletag :search + import Philomena.AsyncUpload import Philomena.CommentsFixtures import Philomena.ImagesFixtures import Philomena.UsersFixtures @@ -14,6 +15,10 @@ defmodule PhilomenaWeb.ImageControllerTest do alias Philomena.Repo alias Philomena.Roles.Role + setup_all do + allow_async_uploads() + end + setup do Search.clear_index!(Image) # :show and :new render the quick tag table, which queries the tags index diff --git a/test/support/async_upload.ex b/test/support/async_upload.ex new file mode 100644 index 000000000..a6799ed35 --- /dev/null +++ b/test/support/async_upload.ex @@ -0,0 +1,36 @@ +defmodule Philomena.AsyncUpload do + @moduledoc """ + Helpers for managing the async image-upload lifecycle from tests. + """ + + @doc """ + Allows supervised image upload tasks to use the current test's sandbox + connection. + """ + def allow_async_uploads do + Ecto.Adapters.SQL.Sandbox.allow( + Philomena.Repo, + self(), + Philomena.ImageUploadSupervisor + ) + + :ok + end + + @doc """ + Waits for all supervised image upload tasks to exit. + """ + def await_async_upload do + for pid <- Task.Supervisor.children(Philomena.ImageUploadSupervisor) do + ref = Process.monitor(pid) + + receive do + {:DOWN, ^ref, :process, ^pid, _reason} -> :ok + after + 5_000 -> raise "async upload process #{inspect(pid)} did not exit" + end + end + + :ok + end +end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 47bfc1293..1f7cd8c17 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -225,33 +225,6 @@ defmodule PhilomenaWeb.ConnCase do %{conn | remote_ip: {10, rem(div(n, 65536), 256), rem(div(n, 256), 256), rem(n, 256)}} end - @doc """ - Waits for the background upload process spawned by a successful image - `:create` to exit. - - A successful `:create` has `Philomena.Images.create_image/2` spawn an - unsupervised upload process that writes to the Repo. Its sandbox allowance - dies with the test process, so wait for it to exit before the test ends; - otherwise it retries with `OwnershipError` every 5s for the rest of the - suite. The endpoint call runs in the test process, so the upload process is - our direct child. - """ - def await_async_upload do - test_pid = self() - - for pid <- Process.list(), Process.info(pid, :parent) == {:parent, test_pid} do - ref = Process.monitor(pid) - - receive do - {:DOWN, ^ref, :process, ^pid, _reason} -> :ok - after - 5_000 -> raise "async upload process #{inspect(pid)} did not exit" - end - end - - :ok - end - @doc """ Helper to set up the conn with global assigns set by the application shell. """