feat: add app_factory for uvicorn multi-worker support#44
Conversation
jfmlima
left a comment
jfmlima
left a comment
There was a problem hiding this comment.
Thanks again @doron1.
Some notes:
The app_factory pattern is solid and I'd like to take it, however, defaulting to 4 workers with SQLite storage is unsafe and will cause database is locked errors under concurrent writes and breaks in-memory auth caching (each worker gets its own AuthStateCache).
Lets ship app_factory with --factory and workers=1 and tackle multi-worker in a follow-up.
Also: #43 should be merged first 👍
| host = os.getenv("HOST", "127.0.0.1") | ||
| port = int(os.getenv("PORT", "8000")) | ||
| debug = os.getenv("DEBUG", "false").lower() == "true" | ||
| workers = int(os.getenv("API_WORKERS", "4")) |
There was a problem hiding this comment.
Default should be "1" not "4" until SQLite concurrency is addressed
There was a problem hiding this comment.
So, obviously you're right. In order for this PR to be complete, it should address the concurrent writes issue (moving away from SQLite? serializing?). So we can either do this one as ground work and address these issues later, or just postpone this PR until there's a fuller solution. This is your baby - your call. Feel free to edit this PR with workers=1 or just discard it to come back to it later. I'm fine either way :-)
| @@ -93,3 +93,6 @@ async def lifespan(app: Litestar) -> AsyncGenerator[None, None]: | |||
|
|
|||
|
|
|||
| app = create_app() | |||
There was a problem hiding this comment.
Since we're going to use app_factory, can we remove this one?
| EXPOSE 8000 | ||
|
|
||
| CMD ["sh", "-c", "uvicorn api.main:app --host ${HOST} --port ${PORT} --log-level info"] | ||
| CMD ["sh", "-c", "uvicorn api.main:app_factory --factory --host ${HOST} --port ${PORT} --log-level info --workers ${API_WORKERS:-4}"] |
2 participants
If you choose to merge this one, a follow-on patch to unraid-templates should also be merged