Make Hue and Spotify setup tasks singletons #16
Conversation
BeeStag
changed the title
backend/dev_requirements.txt
Outdated
There was a problem hiding this comment.
since it's been a while and I can't remember exactly where we left this - I have an upcoming MR that will add a docker compose entry for the frontend service, and I thought it best to put requirement specifications for the frontend and backend in their own respective directories, for the sake of organization. Since these 2 requirements.txt files are both strictly Python module requirements for the backend, I moved this one into the backend directory. But I changed its name so as not to interfere with the backend's dockerfile, and for clarity (since this just-moved file is for dev requirements, and not the bare-bones requirements that the backend's dockerfile cares about).
backend/spotihue/oauth.py
Outdated
|
|
||
| from spotipy import oauth2, util | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| SPOTIFY_OAUTH_TIMEOUT = 60 * 3 # 3 minutes |
There was a problem hiding this comment.
Should we create a .env for this to make it configurable?
| """ | ||
| @wraps(bound_task_function) | ||
| def wrapper(*args, **kwargs): | ||
| me = args[0] |
There was a problem hiding this comment.
Can we add a comment here explaining this line based on our convo?
| @celery_app.task | ||
| def setup_hue(backoff_seconds: int = 5, retries: int = 5) -> None: | ||
| @celery_app.task(base=SingletonTask, bind=True) | ||
| @SingletonTask.run_singleton_task |
There was a problem hiding this comment.
run_singleton_task is static method
UPDATE: the fix for issue 17 is actually changing this line to reference data.data.auth_url. But this PR is still useful.
The Spotify config is crashing because React is running in dev mode, so it's making its API calls twice on purpose to try to screw us up/reveal errors. This revealed that the Spotify setup task (and really, both setup tasks) needs to be idempotent/a singleton task/etc., because while the Hue task's lack of singleton-ness wasn't downright messing it up, the Spotify task couldn't have 2 threads listening on port 8888 at the same time.
changes:
add SingletonTask class (extends celery Task class) - right now its sole purpose is its run_singleton_task decorator (which could just be used as a standalone function, so the @task(base=SingletonTask) param is not necessary at this point for the code to work), but I like having it encapsulated in a class in case we want to extend this class' functionality to accommodate the run_spotihue task (right now SingletonTask does NOT accommodate the run_spotihue task).
add SingletonTaskLock class - keeps track of (task name, task ID) key-value pairs in Redis, with only 1 task ID allowed to "have" a singleton task's name (lock) at a time. its "acquire lock" logic is in a context manager that's used by the SingletonTask.run_singleton_task decorator. inspiration was: https://docs.celeryq.dev/en/latest/tutorials/task-cookbook.html#ensuring-a-task-is-only-executed-one-at-a-time
made the Hue and Spotify setup tasks "bound" tasks (meaning their 1st argument is their own self/instance - even though they don't use self, their decorator does).
make sure the Spotify setup's local socket only listens on 8888 for a couple minutes, so that it can go ahead and peacefully error out and let that Spotify setup task relinquish its lock so that a new Spotify setup task can try again.
oh and also,
right now SingletonTask doesn't accommodate the run_spotihue task because of it being a continually running task with no approximate runtime that can be estimated (the redis lock needs a ttl because not having one will screw things up if the app gets a ctrl-C termination or crashes). I'll probably extend our SingletonTask class logic to add lock refresh-ability at some point, for now I'm pushing this up so that we can fix the Spotify config error.