fastapi / sqlmodel

SQL databases in Python, designed for simplicity, compatibility, and robustness.
https://sqlmodel.tiangolo.com/
MIT License
14.43k stars 658 forks source link

BUG: Background Task Cannot Get Database Updates #220

Open omerXfaruq opened 2 years ago

omerXfaruq commented 2 years ago

First Check

Commit to Help

Example Code

Demo Repo

Description

I encountered an error while working on my side project Memory-Vault. After digging it inside and out I realized that error is due to the sqlmodel itself.

Flow:

➜  SQLModel-Database-Error uvicorn main:app
INFO:     Started server process [41634]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     127.0.0.1:53963 - "POST /user/ HTTP/1.1" 200 OK
tick: 8, user: telegram_chat_id=0 active=True id=1 name='string' gmt=0 scheduled_hours='8,20' , reminders: []
tick: 9, user: telegram_chat_id=0 active=True id=1 name='string' gmt=0 scheduled_hours='8,20' reminders=[] , reminders: []
INFO:     127.0.0.1:53964 - "POST /reminder/ HTTP/1.1" 200 OK    **A Reminder is created at this point with user_id 1**
tick: 10, user: telegram_chat_id=0 active=True id=1 name='string' gmt=0 scheduled_hours='8,20' reminders=[] , reminders: []
tick: 11, user: telegram_chat_id=0 active=True id=1 name='string' gmt=0 scheduled_hours='8,20' reminders=[] , reminders: []
tick: 12, user: telegram_chat_id=0 active=True id=1 name='string' gmt=0 scheduled_hours='8,20' reminders=[] , reminders: []
tick: 13, user: telegram_chat_id=0 active=True id=1 name='string' gmt=0 scheduled_hours='8,20' reminders=[] , reminders: []
^CINFO:     Shutting down
INFO:     Waiting for application shutdown.
INFO:     Application shutdown complete.
INFO:     Finished server process [41634]
➜  SQLModel-Database-Error uvicorn main:app
INFO:     Started server process [41664]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
tick: 1, user: telegram_chat_id=0 active=True id=1 name='string' gmt=0 scheduled_hours='8,20' , reminders: [Reminder(reminder='string1', user_id=1, id=1)]

Operating System

macOS

Operating System Details

No response

SQLModel Version

0.0.3

Python Version

Python 3.7.12

Additional Context

No response

byrman commented 2 years ago

I don't think this is a good idea:

while True:
    await asyncio.sleep(10)
    users = db_read_users(limit=100000)
    for user in users:
        print(f"tick: {tick}, user: {user} , reminders: {user.reminders}")

def db_read_users(
    *,
    only_active_users: bool = True,
    session: Session = next(get_session()),
    offset: int = 0,
    limit: int = 100,
):
    pass

Your default argument is evaluated only once, so each invocation is using the same session, which is never closed. If you do this instead, new reminders will be seen immediately:

while True:
    await asyncio.sleep(10)
    for session in get_session():
        users = db_read_users(session=session, limit=100000)
        for user in users:
            print(f"tick: {tick}, user: {user} , reminders: {user.reminders}")

This might be tempting, but won't properly close your connections (control flow is not resumed after get_session's yield):

while True:
    await asyncio.sleep(10)
    users = db_read_users(session=next(get_session()), limit=100000)
    for user in users:
        print(f"tick: {tick}, user: {user} , reminders: {user.reminders}")
omerXfaruq commented 2 years ago

Hello @byrman thanks for your throughout answer!

I knew it was not advised to use mutable default parameters in python, however I did not know that it was only evaluated once, thanks for sharing that. But I do not understand why the unclosed sessions does not get the updates?

This might be tempting, but won't properly close your connections (control flow is not resumed after get_session's yield): I could not follow this, why does not control flow resume after next(get_session)?

Btw I solved this problem with moving this trigger to an endpoint. It probably was fixed with the correct usage of session ie. session: Session = Depends(get_session)): I tried to mimic that with next(get_session) and it failed miserably resulting with hours of debugging :D

https://github.com/FarukOzderim/Memory-Vault/blob/b5cf3fce0be0ee1eeacc81389d1a1754bcda363b/src/listener.py#L67 https://github.com/FarukOzderim/Memory-Vault/blob/b5cf3fce0be0ee1eeacc81389d1a1754bcda363b/src/events.py#L34

byrman commented 2 years ago

Add a print statement after your get_session's yield statement and see what happens: it won't be reached, because next(get_session) is called only once. In a for loop, next is called until a StopIteration is raised, eventually closing the session (by the context manager).

omerXfaruq commented 2 years ago

Great to learn that @byrman 😸

But why does not an unclosed session receive updates? I feel like it should. Is it a default case for database sessions generally?

byrman commented 2 years ago

Sorry, I didn't investigate that thoroughly. Things like this may happen if there is an outer transaction that is never committed. Other sessions won't see these "dirty" records. But again, I did not dive into it.