Tasks inside LiveView do not run in sandbox about phoenix_ecto HOT 25 CLOSED

And thanks for the nice words, it is no problem :)

from phoenix_ecto.

Does it work for Task.async?

from phoenix_ecto.

No difference. I'll try to create a single-file reproduction script tomorrow, if that helps.

from phoenix_ecto.

I created a Livebook which sets up a sandboxed page that shows this issue:
https://gist.github.com/SteffenDE/80e066d132e2c9b983e4d891d58c4b55

from phoenix_ecto.

You need to setup the sandbox to run in "manual" mode. It typically happens in the test helper. Looking at your code, it doesn't happen anywhere. Perhaps that's the explanation?

You can set ownership_log: :info in the DB configuration to see what the ownership system is reporting.

from phoenix_ecto.

Good to know about the ownership_log, that's indeed useful.

Maybe there is some misunderstanding. I'm having this problem in end to end tests, similarly to what's described in https://dockyard.com/blog/2017/11/15/how-to-add-concurrent-transactional-end-to-end-tests-in-a-phoenix-powered-ember-app. The real tests actually use the POST /sandbox endpoint to get the metadata passed in the User-Agent, the Livebook is just a minimal reproduction of the same phenomenon I'm seeing, without having to setup the whole Playwright + User-Agent stuff I'm actually using. In ExUnit tests where to mode is set to :manual this problem does not occur and Tasks use the sandbox successfully.

I do not see any mention of setting the sandbox to manual mode in the docs for the Phoenix.Ecto.SQL.Sandbox and actually trying to set this when starting the phoenix server leads to a different set of ownership errors.

That's why I was asking if this is supported at all or a known limitation worth documenting.

from phoenix_ecto.

You still need to run in manual mode or shared mode. Without manual mode or shared mode, each process will automatically checkout a connection.

from phoenix_ecto.

Alright, so following the documentation, where should the repo be set to manual or shared mode?

I feel like what is needed here is not possible right now: a kind of automatic mode, but with the option to checkout a connection that transitively allows child processes to use the same connection.

from phoenix_ecto.

Sorry, wrong button pressed...

from phoenix_ecto.

You need to mirror what happens in Phoenix' regular test suites. Which is to boot the app, run test migrations, etc, and then set the sandbox to manual in the test helper. Then for each test you explicitly checkout a connection and potentially set it as shared.

from phoenix_ecto.

In other words, the linked documentation assumes it is being setup on top of the generated Phoenix testing structure. Improvements to the docs are welcome.

from phoenix_ecto.

Hmm, so specifically the part about using external HTTP clients does not read like it assumes the Phoenix testing structure and ExUnit at all. I'm also not sure how I'd set the mode to manual or shared when running concurrent external tests with tools like Cypress or Playwright.

Basically, we're running a full mix release similar to our production environment in Docker, with MIX_ENV=e2e that configures the pool as Ecto.Adapters.SQL.Sandbox and the plug Phoenix.Ecto.SQL.Sandbox enabled. This works perfectly well until a Task is used and it reads like we're just doing it wrong. I created a sample repo to further clarify what I'm trying to achieve that is set-up basically exactly the same as I'm currently using it: https://github.com/SteffenDE/phoenix_playwright_demo

In any case, I want to express my deep appreciation for the work that is done here an in the Elixir community in general. I also feel a little bad that I'm taking up your time with this issue on a Sunday.

from phoenix_ecto.

Does this article help? https://dockyard.com/blog/2017/11/15/how-to-add-concurrent-transactional-end-to-end-tests-in-a-phoenix-powered-ember-app

from phoenix_ecto.

Does this article help? https://dockyard.com/blog/2017/11/15/how-to-add-concurrent-transactional-end-to-end-tests-in-a-phoenix-powered-ember-app

Quoting myself from above:

Maybe there is some misunderstanding. I'm having this problem in end to end tests, similarly to what's described in https://dockyard.com/blog/2017/11/15/how-to-add-concurrent-transactional-end-to-end-tests-in-a-phoenix-powered-ember-app.

It's a great article and actually exactly what inspired me to set this up, but it also does not mention setting the sandbox mode to manual or shared anywhere.

from phoenix_ecto.

Sorry, I missed it. I will take another look again soon. :)

from phoenix_ecto.

Yeah, sorry, I can’t see how this would work otherwise. You need to explicitly allow inside the task to avoid races or set it to manual to allow the $caller fallback to work or set it to shared if you don’t have concurrent tests.

from phoenix_ecto.

Alright so my workaround from above is probably the easiest solution:

@env Mix.env()
def load_data() do
  target = self()

  {:ok, pid} = Task.start(fn ->
    receive do
      :ok -> :ok 
    end
    results = MyApp.Repo.all(...)
    send(target, {:loaded, results})
  end)

  if @env == :e2e do
    Ecto.Adapters.SQL.Sandbox.allow(MyApp.Repo, self(), pid)
  end

  send(pid, :ok)
end

I thought about the following, which would need some (probably major) changes in db_connection: Would it be possible to add something like Ecto.Adapters.SQL.Sandbox.allow_transitive(Repo, self()) to automatically allow Tasks spawned by the owner to use the same connection? Similar to https://github.com/elixir-ecto/db_connection/blob/4d7e789943f1c3a0b54b871f3807a04303e666dc/lib/db_connection/ownership/manager.ex#L286 and somehow detect if the owner set this transitive mode and then actually look at the callers exactly like when using manual mode.

What do you think? If that's not possible, I'll be happy to document this limitation.

from phoenix_ecto.

I am back at the computer now, so I can give more complete answers. Sorry for the brevity previously, I was trying to provide quick feedback to hopefully unblock you but it is clear the problem is more complex. :)

@SteffenDE maybe your workaround would be simpler by allowing in the child?

def load_data() do
  target = self()

  {:ok, pid} = Task.start(fn ->
    if @env == :e2e do
      Ecto.Adapters.SQL.Sandbox.allow(MyApp.Repo, target, self())
    end
    results = MyApp.Repo.all(...)
    send(target, {:loaded, results})
  end)
end

I thought about the following, which would need some (probably major) changes in db_connection: Would it be possible to add something like Ecto.Adapters.SQL.Sandbox.allow_transitive(Repo, self()) to automatically allow Tasks spawned by the owner to use the same connection?

Right, that's pretty much how manual mode works. The whole point is that you need to explicitly configure when the fallback should happen, because you don't want children processes to reuse the parent connection, unless the mode is set to manual.

So, how to set it to manual?

The context you gave about Docker is helpful. I was assuming that in most cases people would still boot the app with mix, where you could do something like mix run --no-halt test/test_helper.exs. In your case, inside a release, the best option is likely to do it in your app start callback in lib/my_app/application.ex. Something like this:

  children = [
    ...,
    {Task, &set_mode_to_manual_on_e2e/0}
  ]

And that will run as the last step of your app booting. :) Let me know if that works or is clear enough.

from phoenix_ecto.

@SteffenDE maybe your workaround would be simpler by allowing in the child?

Oh yes that's much better. I somehow thought that the owner has to allow the other process, but we're passing both pids...

Oh wow, now I feel stupid. I actually tried setting the Repo to manual mode when running in :e2e env, but I did it in the on_mount callback of a Liveview, which lead to the following error in my Playwright tests:

  defp allow_ecto_sandbox(socket) do
    %{assigns: %{phoenix_ecto_sandbox: metadata}} =
      assign_new(socket, :phoenix_ecto_sandbox, fn ->
        if connected?(socket), do: get_connect_info(socket, :user_agent)
      end)

    Ecto.Adapters.SQL.Sandbox.mode(Soda.Repo, :manual)

    if metadata, do: Phoenix.Ecto.SQL.Sandbox.allow(metadata, Ecto.Adapters.SQL.Sandbox)
  end

Request: POST /testapi/eval
** (exit) an exception was raised:
    ** (DBConnection.OwnershipError) cannot find ownership process for #PID<0.1738.0>.

When using ownership, you must manage connections in one
of the four ways:

* By explicitly checking out a connection
* By explicitly allowing a spawned process
* By running the pool in shared mode
* By using :caller option with allowed process

So requests to normal controllers failed. That's why I somehow thought :manual mode would not work when running a full application without ExUnit.

But: your suggestion of setting it as the last step of the main application using a Task works perfectly:

  if Mix.env() == :e2e do
    defp set_manual_mode_on_e2e do
      Ecto.Adapters.SQL.Sandbox.mode(MyApp.Repo, :manual)
    end
  else
    defp set_manual_mode_on_e2e, do: :ok
  end

I'll work on a PR for the docs to mention this step. Is there any reason not to set the repo to manual mode?

Thanks again, you are my hero! :)

from phoenix_ecto.

Is there any reason not to set the repo to manual mode?

Hmm okay so I've found one thing that does not work as I'd like:
Before I set the Repo to manual mode, I could have some tests that run without being sandboxed (simply by not setting the User-Agent string). In manual mode, these unsandboxed tests now lead to Ownership errors as they never explicitly check out a connection. Similarly, a Broadway pipeline that also runs in the e2e env fails with ownership errors too, although I'd simply want it to run unsandboxed.

So seems like this would only work with a Ecto.Adapters.SQL.Sandbox.allow_transitive(Repo, self()) function I've described above.

from phoenix_ecto.

You can split your tests to run the manual and non-manual separately. Alternatively, run the non-manual tests in shared mode by posting to /checkout?shared=true.

from phoenix_ecto.

Yeah I thought about that, but Playwright (the framework we use for performing end-to-end tests) sadly does not offer an easy way to group tests or mark them as synchronous like ExUnit does. Otherwise that would probably be a good solution.

I think I'll go with the workaround for now, running in :auto mode and explicitly allowing the Task. Would you be open for a contribution that adds something like Ecto.Adapters.SQL.Sandbox.allow_transitive(Repo, self())?

from phoenix_ecto.

Yeah I thought about that, but Playwright (the framework we use for performing end-to-end tests) sadly does not offer an easy way to group tests or mark them as synchronous like ExUnit does

Could you split them into two distinct suites?

And what about "Alternatively, run the non-manual tests in shared mode by posting to /checkout?shared=true", wouldn't that work since you already have a checkout?

The issue with allow_transitive is that it would make all automatic checkouts slower... but I am not sure. :)

from phoenix_ecto.

Could you split them into two distinct suites?

I could split the tests into different folders and explicitly tell playwright to first run the sandboxed tests and then later the non-sandboxed, but that's currently more work than the workaround, so I wanted to check other possibilities first.

And what about "Alternatively, run the non-manual tests in shared mode by posting to /checkout?shared=true", wouldn't that work since you already have a checkout?

That would also require me to not run these tests concurrently with the other tests, right? As far as I understand shared mode is not compatible with parallel tests.

The issue with allow_transitive is that it would make all automatic checkouts slower... but I am not sure. :)

Hmm yes I didn't think about that, good point. I'll need some time to explore these options further, but the workaround works fine for me now so it's not a very high priority.

from phoenix_ecto.

That would also require me to not run these tests concurrently with the other tests, right? As far as I understand shared mode is not compatible with parallel tests.

Yeah :(

from phoenix_ecto.