Make actual database request in a thread

[codecov-commenter]

Codecov Report

Merging #50 (e2fe01f) into main (070ec9a) will increase coverage by 2.82%. The diff coverage is 100.00%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #50 +/- ## ========================================== + Coverage 77.37% 80.20% +2.82% ========================================== Files 10 10 Lines 725 798 +73 Branches 108 113 +5 ========================================== + Hits 561 640 +79 + Misses 135 127 -8 - Partials 29 31 +2 ```

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #50 will create unknown performance changes

_{Comparing mattalbr:execute_in_thread (e2fe01f) with main (070ec9a)}

Summary

🆕 1 new benchmarks ⁉️ 1 dropped benchmarks

:warning: _Please fix the performance issues or acknowledge them on CodSpeed._

Benchmarks breakdown

	Benchmark	main	mattalbr:execute_in_thread	Change
🆕	test_load_many_relationships[postgresql]	N/A	103.8 ms	N/A
⁉️	test_hello_world	27.6 µs	N/A	N/A

[erikwrede]

I strongly advise against this. Sessions in SQA are not Thread safe, and this opens a door for concurrent access

[mattalbr]

I strongly advise against this. Sessions in SQA are not Thread safe, and this opens a door for concurrent access

Agreed! This is just a draft PR playing around with things, and I'm still confused why the benchmarks didn't improve (and also sad that they didn't fail with the thread-unsafe error). It's a 6-month tradition for me to forget that sessions aren't thread-safe, but usually I'm reminded by cryptic error messages, last night I managed to remember on my own lol

I'd still like the benchmark to reproduce the bug though. I wonder if the database call is so fast in-memory that there's too much noise and the extra thread actually slowed things down? Maybe I can monkey-patch the call to the database and introduce some delay.

Even though threading isn't the solution, this is a bug that we need to fix, it'll just require an API change. My guess is that the API should take a callable that returns an AsyncSession (which unfortunately are not safe to use in concurrent tasks, hence the factory), and await the call. Do you have thoughts on the API? Should we support a sync session factory as well that we run in an executor or just keep it simple and require async?

Just to be clear, this is a bug because the sync call to the database hogs the event loop stalling all async processing. In our application, it's absolutely decimating our performance.

[erikwrede]

I don't think the executor is the right pattern here. Async session with factory or a Sync Session are the way to go. Using sync sessions with an asgi webserver & Framework should be avoided anyways, usually the executor logic in sync servers should be handled by the webserver imo.

Blocking calls here are totally expected and acceptable for me in this place when using a sync session.