Issue: Thread-safe use of CSqliteExtDatabase update hooks
Author: rokel
State: closed
Created: 2021-04-06 08:40:37

#### Summary

The SQLite hooks like `on_update`, `on_commit` are really useful, thank you for exposing them (and thank you for a fantastic lib!).

But I'm not sure how to use the hooks in a multithreaded program. I have a threadpool of workers servicing a gRPC API that I want to give concurrent read access to a SQLite database. Other than the occasional "database is locked" error, sharing one `CSqliteExtDatabase` instance between threads seems to work well.

However when I introduce the `on_commit` hook I start getting crashes, so I think I am doing something unsafe here. The only way I've found to circumvent these is to create a fresh `CSqliteExtDatabase` instance on each thread as needed. I'm not sure if there are any ramifications for doing that; I'm using the SQLite database as an application file format so it does become a bit complicated to change the `.db` for each one at runtime.

Am I using these hooks in the intended manner?

#### Failing Example

```python
import logging
import random
import concurrent.futures
import time
from playhouse.sqlite_ext import CSqliteExtDatabase

database = CSqliteExtDatabase(
    "test.db",
    autoconnect=False,
    thread_safe=True
)

@database.on_commit
def on_commit():
    pass

class Worker:
    def __init__(self, db: CSqliteExtDatabase):
        self.db = db

    def query(self, timeout):
        with self.db:
            logging.warning(f"starting task (duration={timeout})")
            time.sleep(timeout)
            logging.warning("finished task")

with concurrent.futures.ThreadPoolExecutor() as executor:
    # Start a few 'queries' on different threads
    task_durations = random.sample([d * 0.2 for d in range(1, 10)], 9)

    for duration in task_durations:
        worker = Worker(database)
        executor.submit(worker.query, duration)
```

#### Result

* Windows 10
* Python 3.7.5
* Peewee 3.13.3

```
Windows fatal exception: access violation

Current thread 0x00002308 (most recent call first):
  File "<snip>\venv\lib\site-packages\playhouse\sqlite_ext.py", line 1035 in _close
  File "<snip>\venv\lib\site-packages\peewee.py", line 3059 in close
  File "<snip>\venv\lib\site-packages\peewee.py", line 3015 in __exit__
  File "<snip>/test/test_concurrency.py", line 29 in query
  File "C:\Python37\lib\concurrent\futures\thread.py", line 57 in run
  File "C:\Python37\lib\concurrent\futures\thread.py", line 80 in _worker
  File "C:\Python37\lib\threading.py", line 870 in run
  File "C:\Python37\lib\threading.py", line 926 in _bootstrap_inner
  File "C:\Python37\lib\threading.py", line 890 in _bootstrap
```



So first of all: by default peewee stores connection state in a threadlocal. This means each thread has its own connection. Hooks are specific to the connection they are registered on -- peewee takes care of registering them, but just fyi that's how it works.

Second of all: sqlite cannot accept writes from multiple threads, and unless you're using WAL mode, readers cannot coexist with a writer.

Third of all: because of the gil, only one python thread can be running at any given time.

So using multiple threads with Sqlite is sometimes a misguided effort unless you have a compelling reason.

All that being said, I was able to reproduce a segfault and have a fix ready you can try.

Thanks for the quick response, and for the additional info. I would like to avoid using threads but it's the concurrency model I've inherited from the service lib I'm using (gRPC), and is largely I/O bound so doesn't suffer too much from the GIL I think. I could segregate it off and add another layer inbetween to serialise db access though, but worried about the boilerplate that might entail. Writes are pretty rare for my application at the moment so I'm hoping I can avoid using WAL; trying to avoid multiple files due to use as an application file format.

I pulled 5a55993 and built with `pip install --no-cache-dir ---globaloption=build_ext .` but still get the crash with the test script above. Nevertheless I've tested the 'separate `CSqliteExtDatabase` instance & hooks per-thread' approach and that seems viable so far...



Are you able to get a backtrace? I was able to reproduce a segfault using a slightly different approach and resolved the issue (which had to do with attempting to unregister the hook when the underlying sqlite3 pointer was NULL) - and this is the issue that I patched.