elixir-sqlite / exqlite

An SQLite3 driver for Elixir
https://hexdocs.pm/exqlite
MIT License
217 stars 48 forks source link

Busy ecto sandbox #115

Closed LostKobrakai closed 3 years ago

LostKobrakai commented 3 years ago

@kevinlang suggested I open an issue here in regards to the ecto sandbox testing.

I just tried my example from a few days ago with 0.3.6 and I still get busy errors using the following intentially crafted example:

defmodule Test1Test do
  use PhxSqlite.DataCase, async: true

  test "abc" do
    import Ecto.Query
    query = from m in "mytable", select: %{id: m.id, name: m.name}
    IO.inspect("Start 1")
    now = NaiveDateTime.utc_now()
    Repo.insert_all("mytable", [%{name: "test", inserted_at: now, updated_at: now}])
    Repo.all(query) |> IO.inspect(label: "1")
    Process.sleep(10000)
    Repo.all(query) |> IO.inspect(label: "1")
  end
end

defmodule Test2Test do
  use PhxSqlite.DataCase, async: true

  test "abc" do
    import Ecto.Query
    IO.inspect("Start 2")
    Process.sleep(4000)
    query = from m in "mytable", select: %{id: m.id, name: m.name}
    Repo.all(query) |> IO.inspect(label: "2")

    now = NaiveDateTime.utc_now()
    Repo.insert_all("mytable", [%{name: "test 2", inserted_at: now, updated_at: now}])
  end
end
"Start 1"
1: [%{id: 1, name: "test"}]
"Start 2"
2: []

  1) test abc (Test2Test)
     test/phx_sqlite_web/test_1_test.exs:19
     ** (Exqlite.Error) Database busy
     code: Repo.insert_all("mytable", [%{name: "test 2", inserted_at: now, updated_at: now}])
     stacktrace:
       (ecto_sql 3.5.4) lib/ecto/adapters/sql.ex:751: Ecto.Adapters.SQL.raise_sql_call_error/1
       (ecto_sql 3.5.4) lib/ecto/adapters/sql.ex:660: Ecto.Adapters.SQL.insert_all/8
       (ecto 3.5.8) lib/ecto/repo/schema.ex:54: Ecto.Repo.Schema.do_insert_all/6
       test/phx_sqlite_web/test_1_test.exs:27: (test)

1: [%{id: 1, name: "test"}]
.

Finished in 10.0 seconds
2 tests, 1 failure

It seems the reading does actually work, but writing is the issue here.

kevinlang commented 3 years ago

Can you try again with 0.4.8?

kevinlang commented 3 years ago

Additionally, our adapter does not support async: true. This is because SQLite is single-writer. With the sandbox, each test case is wrapped in a transaction, and SQLite will not be able to do anything until that transaction completes (more or less, there is some additional nuance around read vs write transaction). So if two tests are running at the same time, especially if both are writing to the database in some capacity, one will have to wait until the other test completes, negating the advantage of async.

To translate that into this specific error/case:

The busy_timeout is set to 2000 ms by default. The first case runs first, does an insert which upgrades the transaction to a "write transaction", and then sleeps for 10s. Meanwhile the second tries to run and do another insert, goes into the NIF, and SQLite will keep the call open for ~2seconds waiting for the lock to be released. That does not happen in the 2s time period, and so it returns SQLITE_BUSY.

We plan on adding some better documentation about this and the other limitations of SQLite. Though to be fair only the Postgres adapter supports async AFAIK.

warmwaffles commented 3 years ago

@kevinlang is there a way we can just raise an error with a more descriptive message if async is used? If anything, we should probably add it to the caveat section in the readme.

warmwaffles commented 3 years ago

@LostKobrakai I also just pushed up another fix 0.4.9.

kevinlang commented 3 years ago

I tried looking at the ExUnit docs but couldn't find anything that exposes that async state. Maybe there is something buried away somewhere in the code that is not documented, though.

warmwaffles commented 3 years ago

Either way, we did a bunch of work since that version and 0.4.9 should solve a lot of issues.

LostKobrakai commented 3 years ago

Oh gosh! Sorry about the version used. I meant to use the latest yesterday, but did just blindly mix deps.update exqlite and expected it to update to latest.

@kevinlang thanks for the detailed explanation. So essentially the sandbox only provides automatic cleanup, but doesn't really allow for async tests, at best it supports concurrent and maybe somewhat parallel test. Not really sure however when tests would actually use a read transaction only. There is likely going to be some initial setup going to happen.

We plan on adding some better documentation about this and the other limitations of SQLite. Though to be fair only the Postgres adapter supports async AFAIK.

That would be great. myxql does list something like that here: https://hexdocs.pm/ecto_sql/3.5.4/Ecto.Adapters.MyXQL.html As you can see it doesn't list something in regards to async tests and I just tried out the same testcase in a myxql setup and it does seem to work fine.

LostKobrakai commented 3 years ago

Just a thought, but would having completely separate in memory databases be an option? Instead of isolating on the transaction level isolate on the db level?

warmwaffles commented 3 years ago

Also @LostKobrakai we've split the ecto adapter out of this library last night https://github.com/kevinlang/ecto_sqlite3 we are working to get that under the elixir-sqlite organization.

We can't use an in memory database easily for async stuff. The reason is, db connections are opened and closed frequently during tests so you would need to migrate your database before every test ""

LostKobrakai commented 3 years ago

Thanks for all the information. I guess I'll close this then.