-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tasks inside LiveView do not run in sandbox #157
Comments
Does it work for Task.async? |
No difference. I'll try to create a single-file reproduction script tomorrow, if that helps. |
I created a Livebook which sets up a sandboxed page that shows this issue: |
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 |
Good to know about the 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 I do not see any mention of setting the sandbox to manual mode in the docs for the That's why I was asking if this is supported at all or a known limitation worth documenting. |
You still need to run in manual mode or shared mode. Without manual mode or shared mode, each process will automatically checkout a connection. |
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. |
Sorry, wrong button pressed... |
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. |
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. |
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 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. |
And thanks for the nice words, it is no problem :) |
Quoting myself from above:
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. |
Sorry, I missed it. I will take another look again soon. :) |
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. |
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 What do you think? If that's not possible, I'll be happy to document this limitation. |
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
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
And that will run as the last step of your app booting. :) Let me know if that works or is clear enough. |
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 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
So requests to normal controllers failed. That's why I somehow thought 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! :) |
Hmm okay so I've found one thing that does not work as I'd like: So seems like this would only work with a |
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 |
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 |
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 |
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.
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.
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. |
Yeah :( |
Hello there,
I've setup the
Phoenix.Ecto.SQL.Sandbox
as described here and it works mostly great in our end to end tests.Today I came across the following issue: Our LiveView loads some data asynchronously using
Task.start
:The problem is that the task does not use the sandbox. I thought that the sandbox would use the
:"$callers"
to automatically allow tasks to access it, but I've traced the code and found that this only works when the sandbox runs in:manual
mode, which it does not when using thePhoenix.Ecto.SQL.Sandbox
.Therefore my question: Is there anything that can be done to allow Tasks in the LiveView to use the same sandbox transaction when using
Phoenix.Ecto.SQL.Sandbox
?I've found the following workaround, but I don't like that it introduces extra checks.
If there is nothing that can be done about this, I'd be happy to contribute to the docs about this limitation.
The text was updated successfully, but these errors were encountered: