Skip to content
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

Proper handling of SQLAlchemy database sessions #520

Closed
costrouc opened this issue Aug 2, 2023 · 1 comment · Fixed by #522
Closed

Proper handling of SQLAlchemy database sessions #520

costrouc opened this issue Aug 2, 2023 · 1 comment · Fixed by #522
Labels
impact: high 🟥 This issue affects most of the conda-store users or is a critical issue type: bug 🐛 Something isn't working

Comments

@costrouc
Copy link
Member

costrouc commented Aug 2, 2023

I am convinced this is due to sqlalchemy sessions not being properly handled within conda-store server and workers. This also addresses issues seen in #459. There should be clear boundaries around sessions and they should never be passed between threads since they are not thread-safe https://docs.sqlalchemy.org/en/13/orm/session_basics.html#is-the-session-thread-safe.

Currently main is broken. This was shown clearly by

parent=self, log=self.log, authentication_db=self.conda_store.db
which essentially passes a sqlalchemy session to the Traitlets object conda_store_server.server.auth.Authentication and the value is used across threads (each fastapi route call potentially runs on a separate thread).

A simple fix is to pass the session_factory instead of session:

But this "fix" leads to additional errors about closing sessions in other places. See the following errors message.

emy/orm/attributes.py", line 942, in get
    value = self._fire_loader_callables(state, key, passive)
  File "/home/costrouc/.conda/envs/conda-store-server-dev/lib/python3.10/site-packages/sqlalchemy/orm/attributes.py", line 978, in _fire_loader_callables
    return self.callable_(state, passive)
  File "/home/costrouc/.conda/envs/conda-store-server-dev/lib/python3.10/site-packages/sqlalchemy/orm/strategies.py", line 863, in _load_for_state
    raise orm_exc.DetachedInstanceError(
sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Build at 0x7ff1c197e0e0> is not bound to a Session; lazy load operation of attribute 'specification' cannot proceed (Background on this error at: https://sqlalche.me/e/14/bhk3)

All this is to say that we need to have a clearer boundaries on when a session begins/ends.

The major refactor here is that conda_store_server.app.CondaStore cannot handle the session instance and instead it needs to be passed along from the web request or celery task.

@costrouc costrouc added type: bug 🐛 Something isn't working impact: high 🟥 This issue affects most of the conda-store users or is a critical issue labels Aug 2, 2023
@costrouc
Copy link
Member Author

costrouc commented Aug 3, 2023

Figured out a sneaky bug in conda-store having to do with threads.

from fastapi import Depends

def f():
    return [1, 2, 3]

async def g():
    return [1, 2, 3]

@app.get('/')
def list_items(items: Depends(f))
    return items

The function f is not guaranteed to run in the same thread as list_items but g is. I think this also explains some of the subtle issues with celery being setup on the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: high 🟥 This issue affects most of the conda-store users or is a critical issue type: bug 🐛 Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant